Do not implement HashStable for HashSet (MCP 533) #108312

michaelwoerister · 2023-02-21T14:53:31Z

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.

rustbot · 2023-02-21T14:53:37Z

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-02-21T14:53:40Z

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

michaelwoerister · 2023-02-21T14:54:30Z

Let's make sure this doesn't affect performance:
@bors try @rust-timer queue

bors · 2023-02-21T14:54:39Z

⌛ Trying commit 32266bc9e136e8565aaafa65a1ec90fe74972d9c with merge 2f0c248d7e9de8fbc0a7a26aa2c552ee88ba0389...

bors · 2023-02-21T17:34:59Z

☀️ Try build successful - checks-actions
Build commit: 2f0c248d7e9de8fbc0a7a26aa2c552ee88ba0389 (2f0c248d7e9de8fbc0a7a26aa2c552ee88ba0389)

rust-timer · 2023-02-21T21:32:30Z

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.

RalfJung · 2023-02-23T08:48:27Z

src/tools/miri/src/bin/miri.rs

                    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| {


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?

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.

eholk · 2023-03-08T00:36:22Z

@bors r+

bors · 2023-03-08T00:36:24Z

📌 Commit b79f026 has been approved by eholk

It is now in the queue for this repository.

bors · 2023-03-08T06:07:14Z

⌛ Testing commit b79f026 with merge 9b60e6c...

bors · 2023-03-08T08:50:51Z

☀️ Test successful - checks-actions
Approved by: eholk
Pushing 9b60e6c to master...

rust-timer · 2023-03-08T10:23:57Z

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.

…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](rust-lang/compiler-team#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.

rustbot assigned eholk Feb 21, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 21, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 21, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 21, 2023

RalfJung reviewed Feb 23, 2023

View reviewed changes

michaelwoerister added 7 commits March 1, 2023 10:19

Use DefIdMap instead of FxHashMap for impl_item_implementor_ids query.

5ff00f9

Use LocalDefIdSet instead of FxHashSet for reachable_set query.

f0eadba

Use LocalDefIdSet/Map instead of FxHashSet/Map for live_symbols_and_i…

b0202d9

…gnored_derived_traits query.

Use FxIndexSet instead of FxHashSet for asm_target_features query.

ee8bc5b

Use UnordSet instead of FxHashSet for names_imported_by_glob_use query.

422208a

Remove last instances of HashSet in query result types.

04e5fa3

Do not implement HashStable for HashSet.

b79f026

michaelwoerister force-pushed the hash-set-not-hash-stable branch from 32266bc to b79f026 Compare March 1, 2023 09:21

bors added S-waiting-on-bors 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 Mar 8, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 8, 2023

bors merged commit 9b60e6c into rust-lang:master Mar 8, 2023

rustbot added this to the 1.70.0 milestone Mar 8, 2023

michaelwoerister mentioned this pull request Apr 19, 2023

Create a deterministic FxHashMap wrapper #63713

Closed

jieyouxu mentioned this pull request Aug 14, 2024

Tracking Issue for Reproducible Build bugs and challenges #129080

Open

81 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not implement HashStable for HashSet (MCP 533) #108312

Do not implement HashStable for HashSet (MCP 533) #108312

michaelwoerister commented Feb 21, 2023

rustbot commented Feb 21, 2023

rustbot commented Feb 21, 2023

michaelwoerister commented Feb 21, 2023

This comment has been minimized.

bors commented Feb 21, 2023

bors commented Feb 21, 2023

This comment has been minimized.

rust-timer commented Feb 21, 2023

RalfJung Feb 23, 2023 •

edited

Loading

michaelwoerister Feb 23, 2023

eholk commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

rust-timer commented Mar 8, 2023

Do not implement HashStable for HashSet (MCP 533) #108312

Do not implement HashStable for HashSet (MCP 533) #108312

Conversation

michaelwoerister commented Feb 21, 2023

rustbot commented Feb 21, 2023

rustbot commented Feb 21, 2023

michaelwoerister commented Feb 21, 2023

This comment has been minimized.

bors commented Feb 21, 2023

bors commented Feb 21, 2023

This comment has been minimized.

rust-timer commented Feb 21, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

RalfJung Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

michaelwoerister Feb 23, 2023

Choose a reason for hiding this comment

eholk commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

rust-timer commented Mar 8, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

RalfJung Feb 23, 2023 •

edited

Loading