coverage: Use a separate counter type and simplification step during counter creation #133849
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
A "site" is a node or edge in the coverage graph.
This is more convenient for subsequent patches.
These simplifications are now handled by the transcribe step.
Zalathar
added
the
A-code-coverage
|
rustbot has assigned @petrochenkov. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
This churns all of the coverage tests, so it will conflict with any other PR (e.g. #133089) that re-blesses the |
oli-obk
reviewed
Dec 4, 2024
| } | ||
| } | ||
|
|
||
| fn transcribe_counters(mut self) -> CoverageCounters { |
There was a problem hiding this comment.
So this is basically a normalization if there are no old counters and a system for keeping the diff small if there are old counters?
There was a problem hiding this comment.
This is kind of tricky to explain, but it's also the whole crux of this PR, so I should try.
We can think of this code as going through a series of refactoring stages:
- Graph traversal →
CoverageCounters(status quo) - Graph traversal →
CoverageCounters→Transcriber→ simplifiedCoverageCounters - Graph traversal →
FxHashMap<Site, SiteCounter>→Transcriber→ simplifiedCoverageCounters
The main goal of introducing Transcriber as a middle layer is so that the part before Transcriber can be changed to not be tied to CoverageCounters. To make that feasible, we need to go through the intermediate step of having two different CoverageCounters (old and new), so that we can then replace the first one with something else.
There was a problem hiding this comment.
The fact that final CoverageCounters is simpler than the original one starts off as being a bonus extra, but it also lets the earlier steps not care so much about producing “optimal” results in a single pass. I expect that to be a big help in future changes to how counter creation works.
oli-obk
reviewed
Dec 4, 2024
|
r? @oli-obk |
|
@bors r+ |
bors
added
S-waiting-on-bors
Merged
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 4, 2024
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#133737 (Include LLDB and GDB visualizers in MSVC distribution) - rust-lang#133774 (Make CoercePointee errors translatable) - rust-lang#133831 (Don't try and handle unfed `type_of` on anon consts) - rust-lang#133847 (Remove `-Zshow-span`.) - rust-lang#133849 (coverage: Use a separate counter type and simplification step during counter creation) - rust-lang#133850 (Avoid `opaque type not constrained` errors in the presence of other errors) - rust-lang#133851 (Stop git from merging generated files) - rust-lang#133856 (Update sysinfo version to 0.33.0) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 4, 2024
Rollup merge of rust-lang#133849 - Zalathar:replay, r=oli-obk coverage: Use a separate counter type and simplification step during counter creation When instrumenting a function's MIR for coverage, there is a point where we need to decide, for each node in the control-flow graph, whether its execution count will be tracked by a physical counter, or by an expression that combines physical counters from other parts of the graph. Currently the code for doing that is heavily tied to the final form of the LLVM coverage mapping format, and performs some important simplification steps on-the-fly. These factors make the code extremely difficult to modify without breaking or massively worsening the resulting coverage-instrumentation metadata. --- This PR aims to improve that situation somewhat by adding an extra intermediate representation between the code that chooses how each node will be counted, and the code that converts those decisions into actual tables of physical counters and trees of counter expressions. As part of doing that, some of the simplifications that are currently performed during the main counter creation step have been pulled out into a separate step. In most cases the resulting coverage metadata is equivalent, slightly better, or slightly worse. The biggest outlier is `counters.rs`, where the coverage metadata ends up about 10% larger. This seems to be the result of the new approach having less subexpression sharing (because it relies on flatten-sort-cancel), and therefore being less effective at taking advantage of MIR optimizations to replace counters for unused control-flow with zeroes. I think the modest downside is acceptable in light of the future possibilities opened up by this decoupling.
fmease
added a commit
to fmease/rust
that referenced
this pull request
Dec 9, 2024
coverage: Prefer to visit nodes whose predecessors have been visited In coverage instrumentation, we need to traverse the control-flow graph and decide what kind of counter (physical counter or counter-expression) should be used for each node that needs a counter. The existing traversal order is complex and hard to tweak. This new traversal order tries to be a bit more principled, by always preferring to visit nodes whose predecessors have already been visited, which is a good match for how the counter-creation code ends up dealing with a node's in-edges and out-edges. For several of the coverage tests, this ends up being a strict improvement in reducing the size of the coverage metadata, and also reducing the number of physical counters needed. (The new traversal should hopefully also allow some further code simplifications in the future.) --- This is made possible by the separate simplification pass introduced by rust-lang#133849. Without that, almost any change to the traversal order ends up increasing the size of the expression table or the number of physical counters.
fmease
added a commit
to fmease/rust
that referenced
this pull request
Dec 9, 2024
coverage: Prefer to visit nodes whose predecessors have been visited In coverage instrumentation, we need to traverse the control-flow graph and decide what kind of counter (physical counter or counter-expression) should be used for each node that needs a counter. The existing traversal order is complex and hard to tweak. This new traversal order tries to be a bit more principled, by always preferring to visit nodes whose predecessors have already been visited, which is a good match for how the counter-creation code ends up dealing with a node's in-edges and out-edges. For several of the coverage tests, this ends up being a strict improvement in reducing the size of the coverage metadata, and also reducing the number of physical counters needed. (The new traversal should hopefully also allow some further code simplifications in the future.) --- This is made possible by the separate simplification pass introduced by rust-lang#133849. Without that, almost any change to the traversal order ends up increasing the size of the expression table or the number of physical counters.
fmease
added a commit
to fmease/rust
that referenced
this pull request
Dec 10, 2024
coverage: Prefer to visit nodes whose predecessors have been visited In coverage instrumentation, we need to traverse the control-flow graph and decide what kind of counter (physical counter or counter-expression) should be used for each node that needs a counter. The existing traversal order is complex and hard to tweak. This new traversal order tries to be a bit more principled, by always preferring to visit nodes whose predecessors have already been visited, which is a good match for how the counter-creation code ends up dealing with a node's in-edges and out-edges. For several of the coverage tests, this ends up being a strict improvement in reducing the size of the coverage metadata, and also reducing the number of physical counters needed. (The new traversal should hopefully also allow some further code simplifications in the future.) --- This is made possible by the separate simplification pass introduced by rust-lang#133849. Without that, almost any change to the traversal order ends up increasing the size of the expression table or the number of physical counters.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 10, 2024
Rollup merge of rust-lang#133946 - Zalathar:ready-first, r=oli-obk coverage: Prefer to visit nodes whose predecessors have been visited In coverage instrumentation, we need to traverse the control-flow graph and decide what kind of counter (physical counter or counter-expression) should be used for each node that needs a counter. The existing traversal order is complex and hard to tweak. This new traversal order tries to be a bit more principled, by always preferring to visit nodes whose predecessors have already been visited, which is a good match for how the counter-creation code ends up dealing with a node's in-edges and out-edges. For several of the coverage tests, this ends up being a strict improvement in reducing the size of the coverage metadata, and also reducing the number of physical counters needed. (The new traversal should hopefully also allow some further code simplifications in the future.) --- This is made possible by the separate simplification pass introduced by rust-lang#133849. Without that, almost any change to the traversal order ends up increasing the size of the expression table or the number of physical counters.
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.
When instrumenting a function's MIR for coverage, there is a point where we need to decide, for each node in the control-flow graph, whether its execution count will be tracked by a physical counter, or by an expression that combines physical counters from other parts of the graph.
Currently the code for doing that is heavily tied to the final form of the LLVM coverage mapping format, and performs some important simplification steps on-the-fly. These factors make the code extremely difficult to modify without breaking or massively worsening the resulting coverage-instrumentation metadata.
This PR aims to improve that situation somewhat by adding an extra intermediate representation between the code that chooses how each node will be counted, and the code that converts those decisions into actual tables of physical counters and trees of counter expressions.
As part of doing that, some of the simplifications that are currently performed during the main counter creation step have been pulled out into a separate step.
In most cases the resulting coverage metadata is equivalent, slightly better, or slightly worse. The biggest outlier is
counters.rs, where the coverage metadata ends up about 10% larger. This seems to be the result of the new approach having less subexpression sharing (because it relies on flatten-sort-cancel), and therefore being less effective at taking advantage of MIR optimizations to replace counters for unused control-flow with zeroes. I think the modest downside is acceptable in light of the future possibilities opened up by this decoupling.