coverage: Remove some complex heuristics from counter creation #131398
Conversation
Zalathar
added
the
A-code-coverage
|
rustbot has assigned @matthewjasper. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
A few tests have changes in their coverage reports, but that seems to be a result of them experiencing panics. Coverage instrumentation assumes that panics don't occur, so when a panic does occur, the counts aren't guaranteed to be meaningful, and can be disturbed by implementation details of counter creation. |
When making changes that have a large impact on coverage counter creation, this makes it easier to see whether the number of physical counters has changed. (The highest counter ID seen in coverage maps is not necessarily the same as the number of physical counters actually used by the instrumented code, but it's the best approximation we can get from looking only at the coverage maps, and it should be reasonably accurate in most cases.)
This heuristic makes sense on paper, but currently isn't worth the extra complexity, especially when trying to refactor counter creation in other directions.
The logic behind this traversal order sounds compelling, but in practice it doesn't seem to produce enough benefit to justify its complexity.
Zalathar
force-pushed
the
simplify-counters
branch
from
9b89793
to
9e675b1
Compare
|
On reflection, I want to give this some more thought before merging it. @rustbot author |
rustbot
added
S-waiting-on-author
coverage: Include the highest counter ID seen in `.cov-map` dumps When making changes that have a large impact on coverage counter creation, this makes it easier to see whether the number of physical counters has changed. (The highest counter ID seen in coverage maps is not necessarily the same as the number of physical counters actually used by the instrumented code, but it's the best approximation we can get from looking only at the coverage maps, and it should be reasonably accurate in most cases.) Extracted from rust-lang#131398, since I'm still considering whether to make those changes as-is, whereas this PR is useful and good on its own.
coverage: Include the highest counter ID seen in `.cov-map` dumps When making changes that have a large impact on coverage counter creation, this makes it easier to see whether the number of physical counters has changed. (The highest counter ID seen in coverage maps is not necessarily the same as the number of physical counters actually used by the instrumented code, but it's the best approximation we can get from looking only at the coverage maps, and it should be reasonably accurate in most cases.) Extracted from rust-lang#131398, since I'm still considering whether to make those changes as-is, whereas this PR is useful and good on its own.
Rollup merge of rust-lang#131476 - Zalathar:highest-counter, r=jieyouxu coverage: Include the highest counter ID seen in `.cov-map` dumps When making changes that have a large impact on coverage counter creation, this makes it easier to see whether the number of physical counters has changed. (The highest counter ID seen in coverage maps is not necessarily the same as the number of physical counters actually used by the instrumented code, but it's the best approximation we can get from looking only at the coverage maps, and it should be reasonably accurate in most cases.) Extracted from rust-lang#131398, since I'm still considering whether to make those changes as-is, whereas this PR is useful and good on its own.
This is a very interesting observation. I wonder if it would make sense to introduce another I think there might be value in providing the maximum amount of detail. |
|
Eventually I would love to have better support for accurate coverage in the presence of panics. But there's still a lot to be cleaned up in the current implementation before that becomes feasible, unfortunately. |
When the coverage instrumentor creates coverage counters for various parts of the control-flow graph, it sometimes needs to choose which edges should get a physical counter, and which should get a counter expression computed from other counters.
The current code uses a heuristic based on trying to identify “loop backedges”, which relies on a complex custom graph traversal to identify relevant loop header nodes. That complexity is getting in the way of some deeper changes that I'm trying to make to counter creation, and removing it doesn't seem to make the resulting coverage mappings much worse. In some cases it even makes them better!
This PR therefore removes the edge-expression heuristic and its associated traversal. Now we always choose an arbitrary edge to be given an expression, and we iterate over the graph nodes in ascending ID order.
I've also included a change to the
coverage-dumptool to make it print out the highest counter ID that it sees when printing coverage mappings, as this makes it easier to see whether the number of physical counters has changed.