coverage: Completely overhaul counter assignment, using node-flow graphs #135481
Conversation
This file contains 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
This is similar to the existing `union`, except that bits in the RHS are negated before being incorporated into the LHS. Currently only `DenseBitSet` is supported. Supporting other bitset types is possible, but non-trivial, and currently isn't needed.
Zalathar
added
the
A-code-coverage
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
One of the nice things about this approach, compared to the status-quo, is that we only need to instrument control-flow nodes, not edges. The current code instruments control-flow edges by inserting extra MIR blocks, and then adding counters to those blocks. That works, but it's nice to be able to only instrument nodes, so that the MIR structure isn't disturbed. |
oli-obk
requested changes
Jan 16, 2025
There was a problem hiding this comment.
My third time reading the code in two days. Unfortunately I fail to build a full model of this change in my head. All the code is doing what the comments describe, and the code makes sense in small parts. I have absolutely no idea if it's doing the algorithm from the paper.
Uh... r=me with nits unless you want a reviewer that will actually grok the whole thing.
compiler/rustc_mir_transform/src/coverage/counters/node_flow.rs
Outdated
Show resolved
Hide resolved
|
Added a FIXME, and made |
This avoids having to worry about stack space when traversing very long spantree paths, e.g. when instrumenting a long sequence of if/else statements.
oli-obk
approved these changes
Jan 16, 2025
|
@bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 16, 2025
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#134754 (Implement `use` associated items of traits) - rust-lang#135481 (coverage: Completely overhaul counter assignment, using node-flow graphs) - rust-lang#135504 (Allow coercing safe-to-call target_feature functions to safe fn pointers) - rust-lang#135561 (Update docs for `-Clink-dead-code` to discourage its use) - rust-lang#135574 (ci: mirror ubuntu:22.04 to ghcr.io) - rust-lang#135585 (resolve symlinks of LLVM tool binaries before copying them) - rust-lang#135588 (Add license-metadata.json to rustc-src tarball.) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 17, 2025
Rollup merge of rust-lang#135481 - Zalathar:node-flow, r=oli-obk coverage: Completely overhaul counter assignment, using node-flow graphs The existing code for choosing where to put physical counter-increments gets the job done, but is very ad-hoc and hard to modify without introducing tricky regressions. This PR replaces all of that with a more principled approach, based on the algorithm described in "Optimal measurement points for program frequency counts" (Knuth & Stevenson, 1973). --- We start by ensuring that our graph has “balanced flow”, i.e. each node's flow (execution count) is equal to the sum of all its in-edge flows, and equal to the sum of all its out-edge flows. That isn't naturally true of control-flow graphs, so we introduce a wrapper type `BalancedFlowGraph` to fix that by introducing synthetic nodes and edges as needed. Once our graph has balanced flow, the next step is to create another view of that graph in which each node's successors have all been merged into one “supernode”. Consequently, each node's out-edges can be coalesced into a single out-edge to one of those supernodes. Because of the balanced-flow property, the flow of that coalesced edge is equal to the flow of the original node. Having expressed all of our node flows as edge flows, we can then analyze node flows using techniques for analyzing edge flows. We incrementally build a spanning tree over the merged supernodes, such that each new edge in the spanning tree represents a node whose flow can be computed from that of other nodes. When this is done, we end up with a list of “counter terms” for each node, describing which nodes need physical counters, and how the remaining nodes can have their flow calculated by adding and subtracting those physical counters. --- The re-blessed coverage tests show that this results in modest or major improvements for our test programs. Some tests need fewer physical counters, some tests need fewer expression nodes for the same number of physical counters, and some tests show striking reductions in both.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 18, 2025
…ler-errors coverage: Clean up a few things after the counters overhaul Follow-up to rust-lang#135481. No functional change; this is mostly just deleting or moving code.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 19, 2025
…ler-errors coverage: Clean up a few things after the counters overhaul Follow-up to rust-lang#135481. No functional change; this is mostly just deleting or moving code.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 19, 2025
Rollup merge of rust-lang#135680 - Zalathar:counters-cleanup, r=compiler-errors coverage: Clean up a few things after the counters overhaul Follow-up to rust-lang#135481. No functional change; this is mostly just deleting or moving code.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 24, 2025
coverage: Prepare for upcoming changes to counter creation This is a collection of smaller changes to coverage instrumentation code that have been extracted from a larger PR that I'm still working on, in order to hopefully make review easier. Each individual change should hopefully be mostly self-explanatory. One of the big goals of the upcoming PR will be to defer certain parts of counter-creation until codegen, via the query system, so that ends up being a recurring theme in these changes. Several of the changes are follow-ups to rust-lang#135481. There should be no observable change in compiler output.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jan 24, 2025
coverage: Prepare for upcoming changes to counter creation This is a collection of smaller changes to coverage instrumentation code that have been extracted from a larger PR that I'm still working on, in order to hopefully make review easier. Each individual change should hopefully be mostly self-explanatory. One of the big goals of the upcoming PR will be to defer certain parts of counter-creation until codegen, via the query system, so that ends up being a recurring theme in these changes. Several of the changes are follow-ups to rust-lang#135481. There should be no observable change in compiler output.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 24, 2025
Rollup merge of rust-lang#135873 - Zalathar:be-prepared, r=oli-obk coverage: Prepare for upcoming changes to counter creation This is a collection of smaller changes to coverage instrumentation code that have been extracted from a larger PR that I'm still working on, in order to hopefully make review easier. Each individual change should hopefully be mostly self-explanatory. One of the big goals of the upcoming PR will be to defer certain parts of counter-creation until codegen, via the query system, so that ends up being a recurring theme in these changes. Several of the changes are follow-ups to rust-lang#135481. There should be no observable change in compiler output.
This was referenced Jan 25, 2025
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jan 27, 2025
Incorporate `iter_nodes` into `graph::DirectedGraph` This helper method iterates over all node IDs in the dense range `0..num_nodes`. In practice, we have a lot of graph-algorithm code that already assumes that nodes are densely numbered, by using `num_nodes` to allocate per-node indexed data structures. So I don't think this is actually a substantial change to the de-facto semantics of `graph::DirectedGraph`. --- Resolves a FIXME from rust-lang#135481.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 27, 2025
Rollup merge of rust-lang#136082 - Zalathar:iter-nodes, r=oli-obk Incorporate `iter_nodes` into `graph::DirectedGraph` This helper method iterates over all node IDs in the dense range `0..num_nodes`. In practice, we have a lot of graph-algorithm code that already assumes that nodes are densely numbered, by using `num_nodes` to allocate per-node indexed data structures. So I don't think this is actually a substantial change to the de-facto semantics of `graph::DirectedGraph`. --- Resolves a FIXME from rust-lang#135481.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing code for choosing where to put physical counter-increments gets the job done, but is very ad-hoc and hard to modify without introducing tricky regressions.
This PR replaces all of that with a more principled approach, based on the algorithm described in "Optimal measurement points for program frequency counts" (Knuth & Stevenson, 1973).
We start by ensuring that our graph has “balanced flow”, i.e. each node's flow (execution count) is equal to the sum of all its in-edge flows, and equal to the sum of all its out-edge flows. That isn't naturally true of control-flow graphs, so we introduce a wrapper type
BalancedFlowGraphto fix that by introducing synthetic nodes and edges as needed.Once our graph has balanced flow, the next step is to create another view of that graph in which each node's successors have all been merged into one “supernode”. Consequently, each node's out-edges can be coalesced into a single out-edge to one of those supernodes. Because of the balanced-flow property, the flow of that coalesced edge is equal to the flow of the original node.
Having expressed all of our node flows as edge flows, we can then analyze node flows using techniques for analyzing edge flows. We incrementally build a spanning tree over the merged supernodes, such that each new edge in the spanning tree represents a node whose flow can be computed from that of other nodes.
When this is done, we end up with a list of “counter terms” for each node, describing which nodes need physical counters, and how the remaining nodes can have their flow calculated by adding and subtracting those physical counters.
The re-blessed coverage tests show that this results in modest or major improvements for our test programs. Some tests need fewer physical counters, some tests need fewer expression nodes for the same number of physical counters, and some tests show striking reductions in both.