Computing crate_hash from metadata encoding instead of HIR (implements #94878) - #154724
Open
Daniel-B-Smith wants to merge 2 commits into
Open
Daniel-B-Smith wants to merge 2 commits into
Daniel-B-Smith wants to merge 2 commits into
Conversation
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
Daniel-B-Smith
force-pushed
the
smithdb3/fix-94878
branch
5 times, most recently
from
April 7, 2026 15:59
24c04d5 to
c826667
Compare
This comment has been minimized.
This comment has been minimized.
Daniel-B-Smith
force-pushed
the
smithdb3/fix-94878
branch
from
April 8, 2026 14:22
c826667 to
1fe48e6
Compare
This comment has been minimized.
This comment has been minimized.
Daniel-B-Smith
force-pushed
the
smithdb3/fix-94878
branch
2 times, most recently
from
April 8, 2026 19:34
43a7704 to
338aba3
Compare
This comment has been minimized.
This comment has been minimized.
Daniel-B-Smith
force-pushed
the
smithdb3/fix-94878
branch
2 times, most recently
from
April 8, 2026 21:34
0ddcd96 to
d27cca5
Compare
This comment has been minimized.
This comment has been minimized.
Daniel-B-Smith
force-pushed
the
smithdb3/fix-94878
branch
3 times, most recently
from
April 9, 2026 14:56
1e5f269 to
4171895
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Daniel-B-Smith
changed the title
Contributor
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Daniel-B-Smith
force-pushed
the
smithdb3/fix-94878
branch
from
April 9, 2026 23:55
4171895 to
bb57d62
Compare
This comment has been minimized.
This comment has been minimized.
Daniel-B-Smith
force-pushed
the
smithdb3/fix-94878
branch
from
April 10, 2026 14:25
bb57d62 to
1ad9d87
Compare
Contributor
Author
|
@rustbot ready |
Collaborator
|
Error: Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
bjorn3
reviewed
Aug 17, 2026
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
|
@bors delegate+ r=me with the last commit squashed as discussed in the comments |
Contributor
|
✌️ @Daniel-B-Smith, you can now approve this pull request! If @oli-obk told you to " |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@bors r=oli-obk |
Contributor
This comment has been minimized.
This comment has been minimized.
Contributor
|
💔 Test for b55f73d failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Fixed the issue that blocked the merge. The stub generation code was originally duplicated to handle moving an early return in the code flow inside I've added back the duplicate stub generation. I merged the change into the main commit since the newly added lines are encoder.rs 2521-2534 in commit: e3f246d The comment thread where the duplication was removed for posterity: #154724 (comment) |
Contributor
Author
|
I also added tests/run-make/metadata-stub-incremental-reuse/rmake.rs to confirm the issue was fixed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
The SVH is computed by hashing the entire encoded metadata blob, and SipHash-128 (~2.5 Ir/byte) dominated the cost — it accounted for the whole of the measured regression on metadata-heavy crates. Switch the byte pass to XXH3-128, which is ~10x cheaper per byte on its runtime-detected AVX2 path. XXH3 is non-cryptographic, which is fine here: the SVH is only a change detector, must be reproducible (so it is never keyed secretly, meaning even the previous SipHash use was not collision-resistant), and compiling a crate already executes its build scripts and proc-macros, so the crate author is trusted regardless. The untracked supplements (dep-tracking hash and the HIR hash for e.g. DefKind::GlobalAsm) are still folded in via a StableHasher fingerprint, so coverage is unchanged. twox-hash 2.x is already in the dependency graph via ruzstd; this only adds a direct edge (xxhash3_128 + std features for the runtime SIMD dispatch).
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Contributor
|
@bors r+ |
Contributor
This comment has been minimized.
This comment has been minimized.
Contributor
|
💔 Test for 5ca7eca failed: CI. Failed job:
|
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
☔ The latest upstream changes (presumably #162920) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. This pull request was unapproved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
17 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
This PR converts the crate_hash/SVH to depend on metadata instead of HIR whenever metadata is needed. A trimmed down crate_hash is kept for dylib/binary cases where metadata is not present. It is believed that the metadata will be a more sound way to track dependency changes.
Related to #94878
The metadata hash is calculated on the raw bytes before flush because hashing the elements incrementally turned out to be too expensive.
The change to the HIR hash is potentially safe even without the metadata crate_hash change. Without that change, this PR is a performance regression. I am bundling them because I believe the HIR hash change is more likely to be safe in light of the HIR hash being less load bearing on the SVH.
The dylib/binary crate_hash removes components that I believe are not relevant to those cases. Analysis:
The comment says: "Hash visibility information since it does not appear in HIR." Visibilities only matter to external users of the crate (i.e. consumers of metadata). Without metadata, visibility differences are not observable, so this contributes nothing for incremental correctness.
The comment says: "that content is exported into crate metadata, so any changes to it need to be reflected in the crate hash." Without metadata, there is nothing to export, so this is purely metadata-motivated. The visualizer file path is already covered by HIR (the attribute) and the content is loaded later only if metadata is being written.
These exist solely so that the crate-hash reflects remapped source paths that get embedded into metadata (the comment explicitly says "If we included the full mapping in the SVH, we could only have reproducible builds…"). The remapping itself is already captured via dep_tracking_hash, so for incremental this is redundant.
The crate hash calculation can be reverted via
-Z metadata-crate-hash=no. I manually confirmed that setting that flag generates the same hash as the commit prior to my change. The test for that flag only checks that it changes the value of the hash and does not check the specific value of the hash. I could add a check for the latter, but I'm nervous that a hash golden test would be a maintenance headache.