Change several HashMaps to IndexMap to improve incremental hashing performance

[Kobzol]

Stable hashing hash maps in incremental mode takes a lot of time, especially for some benchmarks like clap. As noted by @Mark-Simulacrum here, this cost could be reduced by replacing some hash maps by indexmaps.

I gathered some statistics and found several hash maps that took a lot of time to hash and replaced them by indexmaps. However, in order for this to work, we need to make sure that these indexmaps have deterministic insertion order. These three are used only in visitors as far as I can see, which seems deterministic. Can we enforce this somehow? Or should some explaining comment be included for these maps?

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[davidtwco]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 8587167dbd6bd39a9e6d8a5ca42d1192b5244c0e with merge 4438132761da5ec0c1b2b3e74658dd8454849393...

[bors]

☀️ Try build successful - checks-actions
Build commit: 4438132761da5ec0c1b2b3e74658dd8454849393 (4438132761da5ec0c1b2b3e74658dd8454849393)

[rust-timer]

Queued 4438132761da5ec0c1b2b3e74658dd8454849393 with parent 56694b0, future comparison URL.

[Mark-Simulacrum]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 8587167dbd6bd39a9e6d8a5ca42d1192b5244c0e with merge 732539524eee94bddaed2893c2b83bc6bfa49d57...

[bors]

☀️ Try build successful - checks-actions
Build commit: 732539524eee94bddaed2893c2b83bc6bfa49d57 (732539524eee94bddaed2893c2b83bc6bfa49d57)

[rust-timer]

Queued 732539524eee94bddaed2893c2b83bc6bfa49d57 with parent 84c2a85, future comparison URL.

[rust-timer]

Finished benchmarking commit (732539524eee94bddaed2893c2b83bc6bfa49d57): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

• Very large improvement in instruction counts (up to -11.8% on incr-full builds of clap-rs)
• Small regression in instruction counts (up to 0.3% on full builds of derive)

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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[davidtwco]

LGTM, don't think we need to be too concerned about deterministic insertion order for these maps, so unless you want to add a comment, r=me.

[Kobzol]

@Mark-Simulacrum told me that we should tread lightly here. @Mark-Simulacrum do you want to add something? :)

[Mark-Simulacrum]

I was under the impression that StableHash needed to be deterministically produced in different builds with the "same" inputs to avoid bugs (or ICEs). This PR is changing a bunch of maps from Fx to the indexset variants, which removes the sort by stable hash key when stablehashing the maps, which I think means we need to otherwise ensure stability through a stable insertion order, right?

Cc @rust-lang/wg-incr-comp

[cjgillot]

I was asking myself something similar earlier. I have similar reservations.

This PR will start tracking the order in which objects are processed. This order is saves in the dep-graph, but does not enter the computation of the query fingerprint for non-anonymous queries.

For instance, in certain cases, like iterations on hir_crate, items are visited in LocalDefId order. This LocalDefId order may leak into the indexmap, and thus into the stable hash.
However, we avoid on purpose any tracking of this LocalDefId order, because it is unstable for incr. comp.

As a consequence, we may introduce spurious query invalidations (best case) or hash verification ICEs (worst case).

(I take the example of LocalDefId ordering because it is known to have caused a few bugs, and because it is easily available incr. comp. unstable information.)

I will think a bit more on it to try and find a solution...

[davidtwco]

r? @cjgillot

[Kobzol]

@cjgillot There is one more thing that I'm not sure how it works. I benchmarked the stable hashing of hash maps, and about half of the cost was invoking ToStableHashKey and creating a vector of keys, and the second half was the sorting itself. For different hash maps and benchmarks it is either bottlenecked by creating the keys or by the sort.

Now, by transforming HashMap to IndexMap, we avoid both the cost of calling ToStableHashKey (because the bound K: HashStable<CTX> is used) and of sorting (because stable hash of index maps doesn't sort).

I wonder, if the key implements HashStable, is it OK to just hash it directly, without invoking ToStableHashKey? I know that there are some outliers like HIR bodies that panic in hash_stable, but I suppose that some key types could "just work"?

• If yes, maybe we could specialize stable hashing of hash maps for keys that can be hashed directly, and not go through ToStableHashKey for these? Then we could avoid some of the cost.
• If not, then using IndexMap here is probably wrong, because ToStableHashKey is not used, while it was used previously?

[cjgillot]

@Kobzol: the issue is about information flow into the stable hash. When iterating over a collection, be it a Vec, a HashMap or an IndexMap, the order of items influences the value of the resulting hash: [a, b] and [b, a] have different hashes.

Meanwhile, there is some information we do not want to track. This is the case of the value of LocalDefId. Inserting a function in a file will change other functions LocalDefId, but it will not change their DefPathHash.

My concern is about controlling this information flow. In order to do that ToStableHashKey trait replaces the iteration order of the collection (which for hash maps is based on the key and the allocated memory capacity and should be irrelevant to the compilation), by the order of a stable hash key (the DefPathHash when the key is LocalDefId). By sorting the vectors by stable key, we manage the information flow.

Using IndexMap, the iteration order is the insertion order. Normally, this insertion order should only depend on tracked information obtained by depending on another query. For instance, a HIR visitor will create a query dependency on hir_owner_nodes, which hashes the in-code declaration order of functions. However, and this is my concern, the order of LocalDefId is freely available without using a query and is purposely untracked.

There are two ways to resolve this concern:

1. avoid depending on the insertion order (= not merging this PR) ;
2. making untracked information inaccessible (= stop implementing Ord for LocalDefId, etc).

In the long run, I think the second solution is the right way forward, but more tricky to reach. For instance, we do not really have a complete list of all untracked information.

In conclusion, I think we should eventually merge this PR, but after for some progress on point 2 above, otherwise we may get incremental hash validation ICEs. However, I may be overly conservative here, since ICEs are still very unlikely.

@Aaron1011 do you agree with this analysis?

[bors]

⌛ Testing commit e475a49 with merge c3d8803baf113e149eea23c0774e7744548fc1b9...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[Kobzol]

Hmm, not really sure what has happened.

[michaelwoerister]

@bors retry

[bors]

⌛ Testing commit e475a49 with merge 1e8c1ed7b34eb2efa04bab53619c6298710b0366...

[bors]

💥 Test timed out

[lqd]

@bors retry

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

⌛ Testing commit e475a49 with merge c9b45e6...

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing c9b45e6 to master...

[rust-timer]

Finished benchmarking commit (c9b45e6): comparison url.

Summary: This benchmark run shows 29 relevant improvements 🎉 but 31 relevant regressions 😿 to instruction counts.

• Arithmetic mean of relevant regressions: 0.3%
• Arithmetic mean of relevant improvements: -0.8%
• Arithmetic mean of all relevant changes: -0.2%
• Largest improvement in instruction counts: -7.5% on incr-full builds of clap-rs check
• Largest regression in instruction counts: 0.6% on full builds of deep-vector check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[rylev]

Given that the performance seen here largely mirrors what was seen when running perf previously (large improvements to clap-rs and otherwise an overall performance wash), I'll mark this as triaged. Let me know if you disagree with this assessment.

@rustbot triage label: perf-regression-triaged