compute and cache HIR hashes at beginning #35854
Conversation
This avoids the compile-time overhead of computing them twice. It also fixes an issue where the hash computed after typeck is differen than the hash before, because typeck mutates the def-map in place. Fixes rust-lang#35549. Fixes rust-lang#35593.
|
(Hmm I may have forgotten the test.) |
Experimentally, this fixes the poor re-use observed in libsyntex-syntax. I'm not sure how to make a regression test for this, though, given the non-deterministic nature of it.
|
|
||
| time(time_passes, | ||
| "assert dep graph", | ||
| move || rustc_incremental::assert_dep_graph(tcx)); | ||
|
|
||
| time(time_passes, | ||
| "serialize dep graph", | ||
| move || rustc_incremental::save_dep_graph(tcx)); | ||
| move || rustc_incremental::save_dep_graph(tcx, &hashes_map)); |
There was a problem hiding this comment.
I'd prefer a less generic name than hashes_map here and above. incr_comp_hashes or ic_hashes_map or ich_map, something that makes it visible that this has to do with incr. comp.
|
I'd be good to include the test case from #35593 (or something more targeted at the underlying issue). |
Yeah, that's easy enough. I should probably try to make a standalone test for the trait-cache issue too. |
| }) | ||
| .collect(); | ||
| candidates.sort(); | ||
| candidates.hash(self.st); |
There was a problem hiding this comment.
This seems like a fine way to make it deterministic. One of the million cases though where I wish we had SmallVec available in the compiler :)
Maybe. A fast allocator though isn't that different. |
This seems like approx a 2x win on syntex_syntax.
This seems not only more correct but allows us to write tests that check whether the krate hash as a whole is clean/dirty
| // except according to those terms. | ||
|
|
||
| // Test incremental compilation tracking where we change field names | ||
| // in between revisions (hashing should be stable). |
There was a problem hiding this comment.
This seems to be a leftover from another test.
|
LGTM! (except for the rogue comment in the test case) |
|
@bors r=mw |
|
📌 Commit 1cc7c90 has been approved by |
|
I added rust-lang-deprecated/rustc-benchmarks#7 to allow us to check that we are still getting reuse in syntex |
compute and cache HIR hashes at beginning This avoids the compile-time overhead of computing them twice. It also fixes an issue where the hash computed after typeck is differen than the hash before, because typeck mutates the def-map in place. Fixes #35549. Fixes #35593. Some performance measurements suggest this `HashesMap` is very small in memory (unobservable via `-Z time-passes`) and very cheap to construct. I do see some (very minor) performance wins in the incremental case after the first run -- the first run costs more because loading the dep-graph didn't have any hashing to do in that case. Example timings from two runs of `libsyntex-syntax` -- the (1) indicates first run, (2) indicates second run, and (*) indicates both together: | Phase | Master | Branch | | ---- | ---- | ---- | | compute_hashes_map (1) | N/A | 0.343 | | load_dep_graph (1) | 0 | 0 | | serialize dep graph (1) | 4.190 | 3.920 | | total (1) | 4.190 | 4.260 | | compute_hashes_map (2) | N/A | 0.344 | | load_dep_graph (2) | 0.592 | 0.252 | | serialize dep graph (2) | 4.119 | 3.779 | | total (2) | 4.71 | 4.375 | | total (*) | 8.9 | 8.635 | r? @michaelwoerister
This avoids the compile-time overhead of computing them twice. It also fixes
an issue where the hash computed after typeck is differen than the hash before,
because typeck mutates the def-map in place.
Fixes #35549.
Fixes #35593.
Some performance measurements suggest this
HashesMapis very small in memory (unobservable via-Z time-passes) and very cheap to construct. I do see some (very minor) performance wins in the incremental case after the first run -- the first run costs more because loading the dep-graph didn't have any hashing to do in that case. Example timings from two runs oflibsyntex-syntax-- the (1) indicates first run, (2) indicates second run, and (*) indicates both together:r? @michaelwoerister