Repository navigation
Conversation
rustbot
added
A-run-make
Author
rustbot
added
S-blocked
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
aerooneqq
reviewed
Oct 9, 2026
| /// Reverse map to the `index` field of each `GenericParamDef`. | ||
| #[stable_hash(ignore)] | ||
| pub param_def_id_to_index: FxHashMap<DefId, u32>, | ||
| pub param_def_id_to_index: ParamDefIdToIndex, |
Contributor
There was a problem hiding this comment.
Can we apply deterministic sorting for all hash maps during metadata encoding (or as a start to all hash maps with keys DefId or LocalDefId)? This solution will fix the issue, however it is not scalable as if other developer will add FxHashMap with DefId key it will be encoded non-determinisitcally.
If there is no good solution except specializations lets use it, maybe we can implement Encodable only for HashMaps with key that implements for example StableHashForMetadata (maybe there is exising trait like StableHash that can be reused) trait and in this case we can apply deterministic encoding for all hashmaps/hashsets.
Author
There was a problem hiding this comment.
open to any changes
I tried to make it automatic and hit coherence. The blanket Encodable for HashMap impl in
rustc_serialize always wins, so I tried out some other things like specialization, default fn on the blanket impl + impls for FxHashMap<DefId, _> and FxHashMap<LocalDefId, _> on EncodeContext, but I saw the feedback on #163321 and stopped, I do think the trait would work alot better in the long run
Contributor
|
I think you could try something along the lines of Then the compiler will complain everywhere where we try to encode a |
This branch has not been deployed
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
5 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.
Fixes #163878
cc @aerooneqq @susitsm
Generics::param_def_id_to_indexis anFxHashMap<DefId, u32>, and it was getting encoded into metadata in whatever order the map happened to iterate. With-ZthreadstheDefIndexes come out in a different order each run, so the keys hash differently and the rmeta bytes flip aroundWent through a few ways to fix this and landed on letting the encoder decide the order: I added a new
TyEncoder::encode_def_id_map.EncodeContextsorts the entries byDefPathHashbefore writing, while the incremental cache and offload manifest just pass through, so they shouldn't cost anything?There's also a new
ParamDefIdToIndexwrapper around the map so theTyEncodablederive routes the field through that method. It implementsDeref+FromIteratorso none of the callers change. And decoding is untouched as wellI picked this over the others attemps I had:
No specialization. My first attempt used
default fn, but per the feedback on UseLocalDefIdas an index in metadata encoding #163321 that's not wanted in encoding code, so I followed theSpanEncoder::encode_spanpattern insteadI use
DefPathHashas the key because it's stable across runs and crates, works for foreignDefIds (the RPITIT-in-impl case can pull in trait params from another crate), and it's what Remap local def ids for deterministic metadata encoding #162809 sorts by anyway when building its remap tableI didn't go with
IndexMapor droppingEncodableforHashMap, since that touches every map in the compiler for one fieldI didn't special-case
Generics. That would fix this field but leave no mechanism for the nextDefId-keyed map in metadataI verified locally on top of #162809: the repro from the issue gives the same md5 across 10 runs with
-Zthreads=2, and the new run-make test passes. On main alone it still varies, because theDefIndexvalues themselves aren't stable yet. So this is blocked on #162809, and the newgenerics-of-issue-163878run-make test will be red until that gets mergedWould appreciate a perf run if possible :)