Refactor ActiveJobGuard#153471
Conversation
61284b1 to
232409f
Compare
|
What is EDIT: Ahh, so it's the |
|
Outside of those suggestions this code looks pretty clean. |
232409f to
1a05470
Compare
|
lol, I don't think I've ever had comments from this many people for a small refactoring PR :) Anyway, I think all comments have now been addressed. |
|
I had intended to move the struct definition when I did the rename, but it got lost during a non-trivial rebase, so I’m happy to see it moved in this PR. |
This comment has been minimized.
This comment has been minimized.
`ActiveJobGuard::complete` and `ActiveJobGuard::drop` have very similar code, though non-essential structural differences obscure this a little. This commit factors out the common code into a new method `ActiveJobGuard::complete_inner`. It also inlines and remove `expect_job`, which ends up having a single call site.
1a05470 to
18ade84
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I rebased. |
|
The label wasn't updated, but I'll assume this is ready. |
…ochenkov Refactor `ActiveJobGuard` A few small improvements. r? @petrochenkov
…uwer Rollup of 13 pull requests Successful merges: - #149130 (Implement coercions between `&pin (mut|const) T` and `&(mut) T` when `T: Unpin`) - #152457 (Pass -pg to linker when using -Zinstrument-mcount) - #153143 (Allow `./x test` to run tests without doc tests and without benchmarks) - #153471 (Refactor `ActiveJobGuard`) - #153595 (`QueryLatch` cleanups) - #153653 (scalable vector: type renames and simple checks) - #152302 (fix: don't suggest replacing `env!("CARGO_BIN_NAME")` with itself) - #153479 (Add rationale for intentional potential_query_instability allows) - #153600 (add test for proc-macros with custom panic payloads) - #153641 (Move `Spanned`.) - #153643 (Avoid projection-only suggestions for inherent assoc types) - #153657 (triagebot: remove myself from some mention groups) - #153659 (Mark an unreachable match arm as such)
…uwer Rollup of 13 pull requests Successful merges: - #149130 (Implement coercions between `&pin (mut|const) T` and `&(mut) T` when `T: Unpin`) - #152457 (Pass -pg to linker when using -Zinstrument-mcount) - #153143 (Allow `./x test` to run tests without doc tests and without benchmarks) - #153471 (Refactor `ActiveJobGuard`) - #153595 (`QueryLatch` cleanups) - #153653 (scalable vector: type renames and simple checks) - #152302 (fix: don't suggest replacing `env!("CARGO_BIN_NAME")` with itself) - #153479 (Add rationale for intentional potential_query_instability allows) - #153600 (add test for proc-macros with custom panic payloads) - #153641 (Move `Spanned`.) - #153643 (Avoid projection-only suggestions for inherent assoc types) - #153657 (triagebot: remove myself from some mention groups) - #153659 (Mark an unreachable match arm as such)
…ochenkov Refactor `ActiveJobGuard` A few small improvements. r? @petrochenkov
…uwer Rollup of 14 pull requests Successful merges: - #149130 (Implement coercions between `&pin (mut|const) T` and `&(mut) T` when `T: Unpin`) - #152457 (Pass -pg to linker when using -Zinstrument-mcount) - #153143 (Allow `./x test` to run tests without doc tests and without benchmarks) - #153471 (Refactor `ActiveJobGuard`) - #153595 (`QueryLatch` cleanups) - #153653 (scalable vector: type renames and simple checks) - #152302 (fix: don't suggest replacing `env!("CARGO_BIN_NAME")` with itself) - #153283 (feat(rustdoc-json): Add optional support for rkyv (de)serialization) - #153479 (Add rationale for intentional potential_query_instability allows) - #153533 (Fix LegacyKeyValueFormat report from docker build: miscellaneous) - #153600 (add test for proc-macros with custom panic payloads) - #153643 (Avoid projection-only suggestions for inherent assoc types) - #153657 (triagebot: remove myself from some mention groups) - #153659 (Mark an unreachable match arm as such)
…uwer Rollup of 13 pull requests Successful merges: - #149130 (Implement coercions between `&pin (mut|const) T` and `&(mut) T` when `T: Unpin`) - #153143 (Allow `./x test` to run tests without doc tests and without benchmarks) - #153471 (Refactor `ActiveJobGuard`) - #153595 (`QueryLatch` cleanups) - #153653 (scalable vector: type renames and simple checks) - #152302 (fix: don't suggest replacing `env!("CARGO_BIN_NAME")` with itself) - #153283 (feat(rustdoc-json): Add optional support for rkyv (de)serialization) - #153479 (Add rationale for intentional potential_query_instability allows) - #153533 (Fix LegacyKeyValueFormat report from docker build: miscellaneous) - #153600 (add test for proc-macros with custom panic payloads) - #153643 (Avoid projection-only suggestions for inherent assoc types) - #153657 (triagebot: remove myself from some mention groups) - #153659 (Mark an unreachable match arm as such)
…uwer Rollup of 13 pull requests Successful merges: - #149130 (Implement coercions between `&pin (mut|const) T` and `&(mut) T` when `T: Unpin`) - #153143 (Allow `./x test` to run tests without doc tests and without benchmarks) - #153471 (Refactor `ActiveJobGuard`) - #153595 (`QueryLatch` cleanups) - #153653 (scalable vector: type renames and simple checks) - #152302 (fix: don't suggest replacing `env!("CARGO_BIN_NAME")` with itself) - #153283 (feat(rustdoc-json): Add optional support for rkyv (de)serialization) - #153479 (Add rationale for intentional potential_query_instability allows) - #153533 (Fix LegacyKeyValueFormat report from docker build: miscellaneous) - #153600 (add test for proc-macros with custom panic payloads) - #153643 (Avoid projection-only suggestions for inherent assoc types) - #153657 (triagebot: remove myself from some mention groups) - #153659 (Mark an unreachable match arm as such)
Rollup merge of #153471 - nnethercote:complete_inner, r=petrochenkov Refactor `ActiveJobGuard` A few small improvements. r? @petrochenkov
…nathanBrouwer Rollup of 13 pull requests Successful merges: - rust-lang#149130 (Implement coercions between `&pin (mut|const) T` and `&(mut) T` when `T: Unpin`) - rust-lang#153143 (Allow `./x test` to run tests without doc tests and without benchmarks) - rust-lang#153471 (Refactor `ActiveJobGuard`) - rust-lang#153595 (`QueryLatch` cleanups) - rust-lang#153653 (scalable vector: type renames and simple checks) - rust-lang#152302 (fix: don't suggest replacing `env!("CARGO_BIN_NAME")` with itself) - rust-lang#153283 (feat(rustdoc-json): Add optional support for rkyv (de)serialization) - rust-lang#153479 (Add rationale for intentional potential_query_instability allows) - rust-lang#153533 (Fix LegacyKeyValueFormat report from docker build: miscellaneous) - rust-lang#153600 (add test for proc-macros with custom panic payloads) - rust-lang#153643 (Avoid projection-only suggestions for inherent assoc types) - rust-lang#153657 (triagebot: remove myself from some mention groups) - rust-lang#153659 (Mark an unreachable match arm as such)
|
@rust-timer build be54f9b |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (be54f9b): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 480.034s -> 479.746s (-0.06%) |
|
This caused the perf regression in #153672 |
|
Regression from messing up inlining I bet |
To fix a small perf regression from rust-lang#153471.
|
I'm trying to fix the perf regression in #153747. The obvious inlining change didn't work, though. |
A few small improvements.
r? @petrochenkov