coverage: Clean up encoding of per-function coverage mapping payloads #115671
Conversation
|
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
There was a problem hiding this comment.
Thanks @Zalathar! It's really great seeing these improvements to the code coverage happen 🙂
I just had one small suggestion for you.
Zalathar
force-pushed
the
mapgen
branch
3 times, most recently
from
7d2963d
to
7418d2e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Zalathar
force-pushed
the
mapgen
branch
3 times, most recently
from
90b0516
to
e1e081a
Compare
|
After trying a few things, I've settled on storing the local-to-global file mappings in an This makes more long-term sense than trying to use zip/enumerate, because when macro-expansion mappings are eventually implemented at some point, there will no longer be a simple 1:1 mapping between filenames and local file IDs. |
This struct was only being used to hold the global file table, and one of its methods didn't even use the table. Changing its methods to ordinary functions makes it easier to see where the table is mutated.
If two or more mappings cover exactly the same region, their relative order will now be preserved from `get_expressions_and_counter_regions`, rather than being disturbed by implementation details of an unstable sort. The current order is: counter mappings, expression mappings, zero mappings. (LLVM will also perform its own stable sort on these mappings, but that sort only compares file ID, start location, and `RegionKind`.)
We already know in advance how many entries will be pushed onto this vector.
Instead of writing coverage mappings into a supplied `&RustString`, this function can just create the buffer itself and return the resulting vector of bytes.
These expressions and counter regions are only needed by the function that encodes a function's coverage mappings payload.
This removes an ad-hoc implementation of `group_by`.
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (725afd2): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 631.821s -> 631.364s (-0.07%) |
This PR contains several small improvements to the code in
rustc_codegen_llvm::coverageinfo::mapgenthat prepares a function's coverage mappings for FFI, and passes them over to LLVM to be encoded into a vector of bytes.These changes are in preparation for some future changes to the coverage implementation, but they should all stand on their own as worthwhile.
There shouldn't be any changes to the resulting coverage mappings, as verified by the existing
tests/coverage-mapandtests/run-coveragesuites.The changes are mostly independent of each other, though they are indirectly affected by the indentation changes made when introducing
GlobalFileTable.