Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Simplify the coverageinfo query #115586

Merged
merged 3 commits into from
Sep 8, 2023
Merged

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Sep 6, 2023

The coverageinfo query walks through a mir::Body's statements to find the total number of coverage counter IDs and coverage expression IDs that have been used, as this information is needed by coverage codegen.

This PR makes 3 nice simplifications to that query:

  • Extract a common iterator over coverage statements, shared by both coverage-related queries
  • Simplify the query's visitor from two passes to just one pass
  • Explicitly track the highest seen IDs in the visitor, and only convert to a count right at the end

I also updated some related comments. Some had been invalidated by these changes, while others had already been invalidated by previous coverage changes.

@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 6, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 6, 2023

The two-pass structure seems to have served some purpose prior to #113428, when we couldn't know for sure whether an operand was a counter ID or expression ID just by looking at it.

That distinction is now preserved explicitly by the Operand enum, so I see no reason to have two passes when just one would suffice.

@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 6, 2023

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Sep 6, 2023
…ents

Both of the coverage queries can now use this one helper function to iterate
over all of the `mir::Coverage` payloads in the statements of a `mir::Body`.
This makes the visitor track the highest seen counter/expression IDs directly,
and only add +1 (to convert to a vector length) at the very end.
@cjgillot
Copy link
Contributor

cjgillot commented Sep 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 7, 2023

📌 Commit e54204c has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2023
@bors
Copy link
Contributor

bors commented Sep 8, 2023

⌛ Testing commit e54204c with merge 69ec430...

@bors
Copy link
Contributor

bors commented Sep 8, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 69ec430 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 8, 2023
@bors bors merged commit 69ec430 into rust-lang:master Sep 8, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 8, 2023
@Zalathar Zalathar deleted the query branch September 8, 2023 05:26
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (69ec430): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
0.8% [0.8%, 0.8%] 1
Regressions ❌
(secondary)
4.1% [1.4%, 6.7%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [0.8%, 0.8%] 1

Cycles

Results

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 628.971s -> 628.962s (-0.00%)
Artifact size: 318.08 MiB -> 318.09 MiB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants