Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make hash of cacheable code the same as if it were uncached #5409

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

dolio
Copy link
Contributor

@dolio dolio commented Oct 10, 2024

This implements the change that throws away the cacheability information when serializing for hashing, so that it doesn't influence the hash.

Also tweaks the interpreter-tests.sh file to accept a ucm on the command line, and falls back to using stack to find the file, which was accidentally changed in the last PR. This will avoid me accidentally changing it in the future.

dolio and others added 3 commits October 10, 2024 14:31
Allow passing in a unison executable on the command line, so the script
doesn't have to be modified to use something other than stack.
This threads version information into the encoder so that when
writing bytes for hashing, it will ignore the cacheability of code,
and produce the same hash as before the caching change.

@unison/internal bump does the same for the jit.
@dolio dolio requested a review from a team as a code owner October 10, 2024 18:38
@pchiusano
Copy link
Member

@dolio @SystemFw before merging this, I'm uncertain how your test in cloud client passed before (that the hashes of things didn't change). It seems like until this PR, the hash of a bunch of things would have changed, and that would have caused your test to fail.

@dolio
Copy link
Contributor Author

dolio commented Oct 10, 2024

What is being hashed? Most stuff didn't change. Only the hash of Code values.

@pchiusano
Copy link
Member

Oh okay, what about the hash of a Unison function? As in:

blah x = x + 938

> crypto.hash Sha3_256 blah

You're saying that never was changed, even before this PR?

@dolio
Copy link
Contributor Author

dolio commented Oct 10, 2024

Right, that is a partial application whose hash is based on the reference for blah and its arguments (but there are 0 in this case). The only thing that would change hash is if you asked for the Code of blah and then hashed it.

@pchiusano pchiusano added the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Oct 10, 2024
@mergify mergify bot merged commit e2c42ca into trunk Oct 10, 2024
17 checks passed
@mergify mergify bot removed the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Oct 10, 2024
@mergify mergify bot deleted the topic/cache-serial branch October 10, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants