Remove special-cased stable hashing for HIR module #92259
Merged
Conversation
This file contains 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
All other 'containers' (e.g. `impl` blocks) hashed their contents in the normal, order-dependent way. However, `Mod` was hashing its contents in a (sort-of) order-independent way. However, the exact order is exposed to consumers through `Mod.item_ids`, and through query results like `hir_module_items`. Therefore, stable hashing needs to take the order of items into account, to avoid fingerprint ICEs. Unforuntately, I was unable to directly build a reproducer for the ICE, due to the behavior of `Fingerprint::combine_commutative`. This operation swaps the upper and lower `u64` when constructing the result, which makes the function non-associative. Since we start the hashing of module items by combining `Fingerprint::ZERO` with the first item, it's difficult to actually build an example where changing the order of module items leaves the final hash unchanged. However, this appears to have been hit in practice in rust-lang#92218 While we're not able to reproduce it, the fact that proc-macros are involved (which can give an entire module the same span, preventing any span-related invalidations) makes me confident that the root cause of that issue is our method of hashing module items. This PR removes all of the special handling for `Mod`, instead deriving a `HashStable` implementation. This makes `Mod` consistent with other 'contains' like `Impl`, which hash their contents through the typical derive of `HashStable`.
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
rustbot
added
the
T-compiler
tmiasko
reviewed
Dec 25, 2021
| let def_path_hash = id.to_stable_hash_key(hcx); | ||
| def_path_hash.0 | ||
| }) | ||
| .fold(Fingerprint::ZERO, |a, b| a.combine_commutative(b)); |
There was a problem hiding this comment.
combine_commutativecan be removed, since it isn't used anywhere else.
There was a problem hiding this comment.
I thought that it might be used again in the future, so I kept it. cc @michaelwoerister
There was a problem hiding this comment.
Yes, let's keep it please. I might have a concrete use for it soon.
There was a problem hiding this comment.
For similar use case that ignores order of combined fingerprints? In that case we should fix the implementation instead of leaving it broken: when used as here it doesn't completely ignore the order when combining more than two fingerprints.
There was a problem hiding this comment.
Opened to #92528 to remove unintentional swap of lower and upper part of the result.
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit da3f196 with merge 07ade458e48a7b28ab8b88f501a29c9d03cac2fe... |
|
☀️ Try build successful - checks-actions |
|
Queued 07ade458e48a7b28ab8b88f501a29c9d03cac2fe with parent f8abed9, future comparison URL. |
|
Finished benchmarking commit (07ade458e48a7b28ab8b88f501a29c9d03cac2fe): comparison url. Summary: This change led to small relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
rustbot
added
perf-regression
| let def_path_hash = id.to_stable_hash_key(hcx); | ||
| def_path_hash.0 | ||
| }) | ||
| .fold(Fingerprint::ZERO, |a, b| a.combine_commutative(b)); |
There was a problem hiding this comment.
Yes, let's keep it please. I might have a concrete use for it soon.
|
@bors r+ |
|
📌 Commit da3f196 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit da3f196 with merge bf286ba644d05e5f5f1369a1edd68ae695ae4a04... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit da3f196 with merge bd7cd2fee41575e5378c11171d0975555c590e5a... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
1 similar comment
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
11 tasks
|
Finished benchmarking commit (2b681ac): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
All other 'containers' (e.g.
implblocks) hashed their contentsin the normal, order-dependent way. However,
Modwas hashingits contents in a (sort-of) order-independent way. However, the
exact order is exposed to consumers through
Mod.item_ids,and through query results like
hir_module_items. Therefore,stable hashing needs to take the order of items into account,
to avoid fingerprint ICEs.
Unforuntately, I was unable to directly build a reproducer
for the ICE, due to the behavior of
Fingerprint::combine_commutative.This operation swaps the upper and lower
u64when constructing theresult, which makes the function non-associative. Since we start
the hashing of module items by combining
Fingerprint::ZEROwiththe first item, it's difficult to actually build an example where
changing the order of module items leaves the final hash unchanged.
However, this appears to have been hit in practice in #92218
While we're not able to reproduce it, the fact that proc-macros
are involved (which can give an entire module the same span, preventing
any span-related invalidations) makes me confident that the root
cause of that issue is our method of hashing module items.
This PR removes all of the special handling for
Mod, instead derivinga
HashStableimplementation. This makesModconsistent with other'contains' like
Impl, which hash their contents through the typicalderive of
HashStable.