perf: Keep dep-graph indices stable and re-emit unchanged nodes - #160214
perf: Keep dep-graph indices stable and re-emit unchanged nodes#160214xmakro wants to merge 1 commit into
Conversation
700cbf4 to
58ab878
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Carry unchanged dep-graph nodes into the next session's file
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5c4fe74): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.1%, secondary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.1%, secondary -1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 489.155s -> 493.076s (0.80%) |
|
r? @cjgillot |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks a lot for the PR. It increases substantially the cognitive complexity of node encoding, and I already have trouble wrapping my head over this.
There was an old design decision around incremental to avoid depending on history: the contents of the dep-graph or on-disk cache should not be modified by the previous dep-graph or cache. This was meant to ease debugging, in particular if a user comes with an ICE after a dozen incremental recompiles.
This PR breaks that design. This may need a MCP to gather thoughts on this.
| self.status.carry_node(prev_index, &self.retained_graph, &mut *local, edges); | ||
| Some(index) | ||
| } | ||
| TrySetColorResult::AlreadyRed => None, |
There was a problem hiding this comment.
Added comment. I think it is the same situation the existing comment in try_mark_previous_green describes:
rust/compiler/rustc_middle/src/dep_graph/graph.rs
Line 1018 in 58b7197
58ab878 to
6006d8d
Compare
6006d8d to
58b7197
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Thank you for the great review, as always. The new stable index invariant also allowed another cleanup: On the history dependence: is the concern the stable indices, or bytes flowing from the old file into the new one? In this PR everything that gets written has passed through decode. Only the edges come from |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
perf: Keep dep-graph indices stable and re-emit unchanged nodes
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e66ed41): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.4%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.0%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.825s -> 491.849s (0.41%) |
View all comments
On a warm rebuild most of the previous graph is still valid. Every one of those nodes is decoded and then encoded again, into a record that differs from the one on disk only because the indices were handed out anew.
In this PR, we keep those nodes at their previous index. A node that is still valid only points at other nodes that are still valid, and those keep their indices too. So its edges do not change, and its whole record is byte for byte the one already on disk. Promoting a node now means writing its old record again instead of building it a second time. Decoding does not keep the header, so we pack it again from the fields we decoded, but the edges are copied across still encoded. Most of what this skips is per edge: scanning the list to pick an index width and writing each index out in that width.
When a node disappears, its index is left unoccupied, and the next session hands unoccupied indices to new nodes before extending the index space. That cannot disturb a promoted record: its edges point only at nodes that were live when the file was written, so nothing in the file references an unoccupied index. Indices a thread takes in a batch and does not use come back the same way, so batch allocation stays as it is.
Stable indices also shrink the color map. A green node's current index equals its previous index, so the map no longer stores it: one byte per node, green, red or unknown, a quarter of its former size.
On disk this costs almost nothing: records exist only for live nodes, so an unoccupied index has no bytes in the file. Measured on a synthetic crate over ten generations of add/remove churn, the dep-graph file is about 1% larger than before and stays flat as nodes come and go. The cost is in memory at load time: the decode tables are sized by the index space, at roughly 50 bytes per unoccupied index, and the index space tracks the most nodes the graph has ever held at once.
Much of the motivation of this PR is what stable indices allow as next perf improvements:
-Zquery-dep-graphreads that list, so the walk can stop building it otherwise.