Use drop_guard in some places in {core,alloc,std} - #161702
Conversation
|
r? @clarfonthey rustbot has assigned @clarfonthey. Use Why was this reviewer chosen?The reviewer was selected based on:
|
b733c5d to
a5a9d5d
Compare
|
The way the code was moved around makes this a bit more difficult to review, but otherwise, it looks good to me. Just a limitation of how diffs work. I think this is a strict improvement regardless of stabilisation. I don't think this should require more than a rebase from Yosh, so, it should be fine that this conflicts with the stabilisation PR. @bors r+ rollup Thank you! |
…they
Use `drop_guard` in some places in {core,alloc,std}
- Tracking issue: rust-lang#144426
- Will conflict with rust-lang#161520
- rust-lang#161550 would also be cool occasionally
Didn't touch the places where manual `impl Drop`s had `#[inline]` on their `fn drop` or where the guard type had other `impl`s beside `Drop` and/or was named a lot.
No LLMs used, only pure human slop.
Rollup of 9 pull requests Successful merges: - #160533 (dirfd dir operations (3/4)) - #161294 (add crashtests [6/N]) - #160392 (library: motor: bump moto-rt ABI ver to 17) - #161646 (interpret: fix after_local_read handling) - #160819 (Rework `div_ceil` for nonzero integers) - #161591 (run `classify-runtime-const` test for `f128`) - #161680 (renovate: add lockfile update warning) - #161702 (Use `drop_guard` in some places in {core,alloc,std}) - #161709 (Stop rustfmt deleting attributes in fn params)
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #161715 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#161715), which was unapproved. |
a5a9d5d to
e990441
Compare
|
@rustbot ready |
|
@GrigorenkoPV thank you for filing this PR; this is really cool to see and a very helpful reference for how |
Not sure how accurate that metric is, but yeah, code reduction is definitely real. It is especially nice to not have to write all the types out again for the struct fields. And the generics for both the struct and the Drop impl (since those cannot be reused from the outer scope; in one place the old approach even required a
Absolutely. I remember touching |
This comment has been minimized.
This comment has been minimized.
e990441 to
f6e2586
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Apologies for forcing you to rebate again .-. |
3546f3a to
4f404c1
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. |
|
The grindset never stops @rustbot ready |
|
@bors try jobs=dist-various-1,dist-various-2,test-various,x86_64-msvc-1,dist-x86_64-linux-quick |
This comment has been minimized.
This comment has been minimized.
Use `drop_guard` in some places in {core,alloc,std}
try-job: dist-various-1
try-job: dist-various-2
try-job: test-various
try-job: x86_64-msvc-1
try-job: dist-x86_64-linux-quick
|
@bors r+ |
…they
Use `drop_guard` in some places in {core,alloc,std}
- Tracking issue: rust-lang#144426
- Will conflict with rust-lang#161520
- rust-lang#161550 would also be cool occasionally
Didn't touch the places where manual `impl Drop`s had `#[inline]` on their `fn drop` or where the guard type had other `impl`s beside `Drop` and/or was named a lot.
No LLMs used, only pure human slop.
…uwer Rollup of 8 pull requests Successful merges: - #137720 (support `#[target_feature(enable = ...)]` on `#[naked]` functions) - #160923 (Handle multiple action records in EH personality function) - #161788 (Check to ensure we're running against the correct LLVM version) - #161644 (bootstrap: Flatten and rename `compute_src_directory_via_git`) - #161702 (Use `drop_guard` in some places in {core,alloc,std}) - #161931 (Revert "Add rustc_test_entrypoint_marker") - #162015 (touch up "get attribute" docs.) - #162019 (remove a couple of redundant clones, thanks clippy)
…they
Use `drop_guard` in some places in {core,alloc,std}
- Tracking issue: rust-lang#144426
- Will conflict with rust-lang#161520
- rust-lang#161550 would also be cool occasionally
Didn't touch the places where manual `impl Drop`s had `#[inline]` on their `fn drop` or where the guard type had other `impl`s beside `Drop` and/or was named a lot.
No LLMs used, only pure human slop.
…they
Use `drop_guard` in some places in {core,alloc,std}
- Tracking issue: rust-lang#144426
- Will conflict with rust-lang#161520
- rust-lang#161550 would also be cool occasionally
Didn't touch the places where manual `impl Drop`s had `#[inline]` on their `fn drop` or where the guard type had other `impl`s beside `Drop` and/or was named a lot.
No LLMs used, only pure human slop.
…uwer Rollup of 11 pull requests Successful merges: - #160923 (Handle multiple action records in EH personality function) - #161788 (Check to ensure we're running against the correct LLVM version) - #158303 (Always escape grapheme extenders in `str::escape_debug`) - #161644 (bootstrap: Flatten and rename `compute_src_directory_via_git`) - #161702 (Use `drop_guard` in some places in {core,alloc,std}) - #161713 (Add config for gram editor to .gitignore) - #161792 (Fix and improve diagnostics for lint `rust_2021_prefixes_incompatible_syntax`) - #161931 (Revert "Add rustc_test_entrypoint_marker") - #162015 (touch up "get attribute" docs.) - #162019 (remove a couple of redundant clones, thanks clippy) - #162023 (Remove redundant braces from `NonZero` doctests)
…uwer Rollup of 11 pull requests Successful merges: - #160923 (Handle multiple action records in EH personality function) - #161788 (Check to ensure we're running against the correct LLVM version) - #158303 (Always escape grapheme extenders in `str::escape_debug`) - #161644 (bootstrap: Flatten and rename `compute_src_directory_via_git`) - #161702 (Use `drop_guard` in some places in {core,alloc,std}) - #161713 (Add config for gram editor to .gitignore) - #161792 (Fix and improve diagnostics for lint `rust_2021_prefixes_incompatible_syntax`) - #161931 (Revert "Add rustc_test_entrypoint_marker") - #162015 (touch up "get attribute" docs.) - #162019 (remove a couple of redundant clones, thanks clippy) - #162023 (Remove redundant braces from `NonZero` doctests)
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (a643d98): comparison URL. Overall result: ❌ regressions - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression 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 1.1%, secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.3%, secondary 2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 1.2%, secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.19s -> 473.278s (-0.40%) |
|
perf triage: Hi! This looks like it caused a pretty broad perf regression in #162028. This is quite unexpected for a PR that is just a cleanup (I assume), even though it technically makes sense if you look at the detailed results. Can we reduce the impact of this somehow? This is broad enough that I'd consider a revert, but if we have some idea to reduce the impact quickly in a followup, that'd probably be nicer (it looks like this is pretty bitrotty, looking at the history here). It looks like this is from more type instantiations and downstream codegen queries, but I wonder whether some changes here are more impactul than others. |
yoshuawuyts
left a comment
There was a problem hiding this comment.
RE: perf regressions - right now DropGuard doesn't have #[inline] on the Drop::drop impl. This patch did replace a number of those in critical data structures (BTree, VecDeque, Slice). Maybe a first good candidate to try out would be to add #[inline] to the Drop impl and then check whether that affects the perf?
| // Run the destructor of the value even if the destructor of the key panics. | ||
| struct Dropper<'a, T>(&'a mut MaybeUninit<T>); | ||
| impl<T> Drop for Dropper<'_, T> { | ||
| #[inline] |
There was a problem hiding this comment.
For the perf regressions, this one stands out to me
| // Dropping `guard` handles moving the remaining elements into place. | ||
| impl<'r, 'a, T, A: Allocator> Drop for DropGuard<'r, 'a, T, A> { | ||
| #[inline] | ||
| fn drop(&mut self) { |
There was a problem hiding this comment.
This one stands out to me as well.
| num_init: usize, | ||
| } | ||
| impl<'a, T, A: Allocator> Drop for DropGuard<'a, T, A> { | ||
| #[inline] |
View all comments
core::mem::DropGuard#144426core::mem::DropGuard#161520DropGuard::new(|| ...)#161550 would also be cool occasionallyDidn't touch the places where manual
impl Drops had#[inline]on theirfn dropor where the guard type had otherimpls besideDropand/or was named a lot.No LLMs used, only pure human slop.