Do not implement HashStable for HashSet (MCP 533) by michaelwoerister · Pull Request #108312 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits7 Checks0 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
This PR removes all occurrences of HashSet in query results, replacing it either with FxIndexSet or with UnordSet, and then removes the HashStable implementation of HashSet. This is part of implementing MCP 533, that is, removing the HashStable implementations of all collection types with unstable iteration order.
The changes are mostly mechanical. The only place where additional sorting is happening is in Miri's override implementation of the exported_symbols query.
r? @eholk
(rustbot has picked a reviewer for you, use r? to override)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Some changes occurred to MIR optimizations
cc @rust-lang/wg-mir-opt
Some changes occurred in src/tools/clippy
cc @rust-lang/clippy
The Miri subtree was changed
cc @rust-lang/miri
This comment has been minimized.
⌛ Trying commit 32266bc9e136e8565aaafa65a1ec90fe74972d9c with merge 2f0c248d7e9de8fbc0a7a26aa2c552ee88ba0389...
☀️ Try build successful - checks-actions
Build commit: 2f0c248d7e9de8fbc0a7a26aa2c552ee88ba0389 (2f0c248d7e9de8fbc0a7a26aa2c552ee88ba0389)
This comment has been minimized.
Finished benchmarking commit (2f0c248d7e9de8fbc0a7a26aa2c552ee88ba0389): comparison URL.
Overall result: no relevant changes - no action needed
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 may lead to changes in compiler perf.
@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 2.2% | [2.2%, 2.2%] | 1 |
| Regressions ❌ (secondary) | 4.0% | [3.4%, 4.6%] | 2 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -2.4% | [-2.6%, -2.2%] | 2 |
| All ❌✅ (primary) | 2.2% | [2.2%, 2.2%] | 1 |
Cycles
This benchmark run did not return any relevant results for this metric.
| tcx.arena.alloc_from_iter( |
|---|
| // This is based on: |
| // https://github.com/rust-lang/rust/blob/2962e7c0089d5c136f4e9600b7abccfbbde4973d/compiler/rustc\_codegen\_ssa/src/back/symbol\_export.rs#L62-L63 |
| // https://github.com/rust-lang/rust/blob/2962e7c0089d5c136f4e9600b7abccfbbde4973d/compiler/rustc\_codegen\_ssa/src/back/symbol\_export.rs#L174 |
| tcx.reachable_set(()).iter().filter_map(|&local_def_id |
| reachable_set.into_iter().filter_map(|&local_def_id |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place where additional sorting is happening is in Miri's override implementation of the exported_symbols query.
This was copied from elsewhere in rustc (specifically reachable_non_generics_provider), so if the change only happens in Miri but not the source where we copied this from, that seems suspicious?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type of reachable_non_generics_provider is a DefIdMap, i.e. a type that does not have a visible iteration order, so no sorting needs to happen there.
rustc's version of exported_symbols already does some stable sorting before returning the slice, so that is fine too. That Miri didn't sort the slice before returning could be considered a bug because queries must be deterministic across compilation sessions (and the iteration order of a HashSet of DefId/DefIndex/LocalDefId isn't). It's unlikely that this can cause a problem if incremental compilation isn't involved though.
…gnored_derived_traits query.
📌 Commit b79f026 has been approved by eholk
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Finished benchmarking commit (9b60e6c): comparison URL.
Overall result: ❌ regressions - no action needed
@rustbot label: -perf-regression
Instruction count
This is a highly reliable metric that was used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 0.4% | [0.4%, 0.4%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Max RSS (memory usage)
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | -2.1% | [-2.9%, -1.3%] | 2 |
| All ❌✅ (primary) | - | - | 0 |
Cycles
This benchmark run did not return any relevant results for this metric.
flip1995 pushed a commit to flip1995/rust that referenced this pull request
…table, r=eholk
Do not implement HashStable for HashSet (MCP 533)
This PR removes all occurrences of HashSet in query results, replacing it either with FxIndexSet or with UnordSet, and then removes the HashStable implementation of HashSet. This is part of implementing MCP 533, that is, removing the HashStable implementations of all collection types with unstable iteration order.
The changes are mostly mechanical. The only place where additional sorting is happening is in Miri's override implementation of the exported_symbols query.
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.