Skip to content

Conversation

Zalathar
Copy link
Contributor

A small and self-contained improvement, extracted from some larger changes that I'm still working on.

Ultimately I want to avoid creating these sum counter-expressions in some cases (in favour of just adding physical counters directly to the nodes we care about), so a good incremental move towards that is splitting the “gather edge counters” step out from the ”build a sum of those counters” step.

Creating an extra intermediate vector should have negligible cost (and coverage isn't exercised by the benchmark suite anyway). The removed logging is redundant with the #[instrument(..)] logging we already have on the underlying method calls.

@Zalathar Zalathar added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Sep 12, 2024
Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest taking an iterator instead of forcing a collect. Unless you really need this to be a slice for some followup work. but even then, you could move the .iter().cloned() to the caller.

@Swatinem
Copy link
Contributor

I guess I should have read the description more carefully 🙈 don’t mind me then, as it looks like you will be needing a Vec in the future here anyway.

@Zalathar
Copy link
Contributor Author

Yeah, creating the intermediate vector is deliberate, so that the two steps are clearly separate (and can be pulled further apart without additional hassle).

@Zalathar
Copy link
Contributor Author

And while I could make the helper take an iterator instead of a slice, I prefer the direct simplicity of the slice.

It would be easy enough to change that back to taking an iterator if necessary, but I don't foresee that happening in the near future.

@Zalathar
Copy link
Contributor Author

Hmm, was rustbot asleep?

r? compiler

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 12, 2024
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 12, 2024

📌 Commit 2344133 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 12, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 12, 2024
coverage: Simplify creation of sum counters

A small and self-contained improvement, extracted from some larger changes that I'm still working on.

Ultimately I want to avoid creating these sum counter-expressions in some cases (in favour of just adding physical counters directly to the nodes we care about), so a good incremental move towards that is splitting the “gather edge counters” step out from the ”build a sum of those counters” step.

Creating an extra intermediate vector should have negligible cost (and coverage isn't exercised by the benchmark suite anyway). The removed logging is redundant with the `#[instrument(..)]` logging we already have on the underlying method calls.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#130101 (some const cleanup: remove unnecessary attributes, add const-hack indications)
 - rust-lang#130208 (Introduce `'ra` lifetime name.)
 - rust-lang#130263 (coverage: Simplify creation of sum counters)
 - rust-lang#130273 (more eagerly discard constraints on overflow)
 - rust-lang#130276 (Add test for nalgebra hang in coherence)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7cae463 into rust-lang:master Sep 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
Rollup merge of rust-lang#130263 - Zalathar:sums, r=compiler-errors

coverage: Simplify creation of sum counters

A small and self-contained improvement, extracted from some larger changes that I'm still working on.

Ultimately I want to avoid creating these sum counter-expressions in some cases (in favour of just adding physical counters directly to the nodes we care about), so a good incremental move towards that is splitting the “gather edge counters” step out from the ”build a sum of those counters” step.

Creating an extra intermediate vector should have negligible cost (and coverage isn't exercised by the benchmark suite anyway). The removed logging is redundant with the `#[instrument(..)]` logging we already have on the underlying method calls.
@Zalathar Zalathar deleted the sums branch September 12, 2024 22:25
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) 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.

6 participants