replace vec::Drain drop loops with drop_in_place #85157

the8472 · 2021-05-10T20:02:05Z

The Drain::drop implementation came up in #82185 (comment) as potentially interfering with other optimization work due its widespread use somewhere in println!

@rustbot label T-libs-impl

rust-highfive · 2021-05-10T20:02:09Z

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2021-05-10T20:07:36Z

@bors try @rust-timer queue

rust-timer · 2021-05-10T20:07:37Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-05-10T20:07:45Z

⌛ Trying commit 5cd25a3c8de8ac50571849c91869d27b0aedf1b4 with merge f43378a4f09d5d51bfbc4bca4d5abbd28238f176...

bors · 2021-05-10T20:24:23Z

💔 Test failed - checks-actions

library/alloc/src/vec/drain.rs

Mark-Simulacrum · 2021-05-10T21:42:06Z

@bors try

bors · 2021-05-10T21:42:14Z

⌛ Trying commit 032fa35 with merge 29e55f8f91bd74ab69933dd9166d41317e0146b7...

bors · 2021-05-10T22:28:25Z

☀️ Try build successful - checks-actions
Build commit: 29e55f8f91bd74ab69933dd9166d41317e0146b7 (29e55f8f91bd74ab69933dd9166d41317e0146b7)

rust-timer · 2021-05-10T22:28:27Z

Queued 29e55f8f91bd74ab69933dd9166d41317e0146b7 with parent 6fd7a6d, future comparison URL.

rust-timer · 2021-05-11T02:39:40Z

Finished benchmarking try commit (29e55f8f91bd74ab69933dd9166d41317e0146b7): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

the8472 · 2021-05-11T07:57:55Z

seems kind of mixed. @SkiFire13 do you think it would make sense to test your changes on top of these?

SkiFire13 · 2021-05-11T08:15:19Z

I might give it a try, give me a sec to rebase my pr on top of yours.

JohnCSimon · 2021-11-08T14:23:39Z

triage:
@yaahc This has been sitting in review state since may

yaahc · 2021-11-15T19:54:11Z

triage: @yaahc This has been sitting in review state since may

Still on my radar, just haven't had a chance to review it yet.

library/alloc/src/vec/drain.rs

Mark-Simulacrum · 2021-12-07T23:00:15Z

@the8472 if you want to leave the cleanup of spare capacity to something simpler for the future, that seems OK to me as well I think.

Mark-Simulacrum · 2021-12-09T14:28:26Z

@bors r+

bors · 2021-12-09T14:28:27Z

📌 Commit 2d8a11b has been approved by Mark-Simulacrum

bors · 2021-12-09T15:01:45Z

⌛ Testing commit 2d8a11b with merge 0b42dea...

bors · 2021-12-09T17:50:53Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 0b42dea to master...

rust-timer · 2021-12-09T19:21:14Z

Finished benchmarking commit (0b42dea): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

Large improvement in instruction counts (up to -1.7% on full builds of match-stress-enum)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the8472 · 2021-12-09T19:27:17Z

Nice, better results than the previous perf run.

RalfJung · 2021-12-11T03:34:46Z

Is it possible that this introduced a use-after-free?

RalfJung · 2021-12-11T03:37:24Z

library/alloc/src/vec/drain.rs

- mem::forget(guard);
+ let iter = mem::replace(&mut self.iter, (&mut []).iter());
+ let drop_len = iter.len();
+ let drop_ptr = iter.as_slice().as_ptr();


Specifically, Miri says that in this line as_slice points to dangling memory (in some cases, here triggered by the test_replace_range testcase)

I'm under the impression that the iter should be valid at all times, but I'll have a closer look at the test-case.

(Continuing in #91772)

rust-highfive assigned yaahc May 10, 2021

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 10, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2021

SkiFire13 reviewed May 10, 2021

View reviewed changes

library/alloc/src/vec/drain.rs Outdated Show resolved Hide resolved

the8472 force-pushed the drain-drop-in-place branch from 5cd25a3 to 032fa35 Compare May 10, 2021 21:24

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 11, 2021

SkiFire13 mentioned this pull request May 11, 2021

Specialize Iterator for &mut I where I: Sized #82185

Closed

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 31, 2021

JohnTitor removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 15, 2021

Mark-Simulacrum reviewed Nov 19, 2021

View reviewed changes

library/alloc/src/vec/drain.rs Outdated Show resolved Hide resolved

Mark-Simulacrum reviewed Nov 19, 2021

View reviewed changes

library/alloc/src/vec/drain.rs Show resolved Hide resolved

the8472 added 2 commits November 20, 2021 01:12

replace vec::Drain drop loops with drop_in_place

ce99402

document why we're not directly passing drop_ptr to drop_in_place

6851b8d

the8472 force-pushed the drain-drop-in-place branch from 032fa35 to 6851b8d Compare November 20, 2021 00:29

Mark-Simulacrum assigned Mark-Simulacrum and unassigned yaahc Nov 20, 2021

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2021

Use *mut [T] instead of [MaybeUninit<T>]

2d8a11b

the8472 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 8, 2021

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 Dec 9, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2021

bors merged commit 0b42dea into rust-lang:master Dec 9, 2021

rustbot added this to the 1.59.0 milestone Dec 9, 2021

bors mentioned this pull request Dec 9, 2021

Implement TrustedRandomAccess for vec::Drain #81617

Closed

RalfJung reviewed Dec 11, 2021

View reviewed changes

camelid mentioned this pull request Dec 11, 2021

Use-after-free in Drop impl for vec::Drain #91772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace vec::Drain drop loops with drop_in_place #85157

replace vec::Drain drop loops with drop_in_place #85157

the8472 commented May 10, 2021 •

edited

Loading

rust-highfive commented May 10, 2021

Mark-Simulacrum commented May 10, 2021

rust-timer commented May 10, 2021

bors commented May 10, 2021

This comment has been minimized.

This comment has been minimized.

bors commented May 10, 2021

Mark-Simulacrum commented May 10, 2021

bors commented May 10, 2021

bors commented May 10, 2021

rust-timer commented May 10, 2021

rust-timer commented May 11, 2021

the8472 commented May 11, 2021

SkiFire13 commented May 11, 2021

JohnCSimon commented Nov 8, 2021

yaahc commented Nov 15, 2021

Mark-Simulacrum commented Dec 7, 2021

Mark-Simulacrum commented Dec 9, 2021

bors commented Dec 9, 2021

bors commented Dec 9, 2021

bors commented Dec 9, 2021

rust-timer commented Dec 9, 2021

the8472 commented Dec 9, 2021

RalfJung commented Dec 11, 2021

RalfJung Dec 11, 2021

the8472 Dec 11, 2021

RalfJung Dec 11, 2021

replace vec::Drain drop loops with drop_in_place #85157

replace vec::Drain drop loops with drop_in_place #85157

Conversation

the8472 commented May 10, 2021 • edited Loading

rust-highfive commented May 10, 2021

Mark-Simulacrum commented May 10, 2021

rust-timer commented May 10, 2021

bors commented May 10, 2021

This comment has been minimized.

This comment has been minimized.

bors commented May 10, 2021

Mark-Simulacrum commented May 10, 2021

bors commented May 10, 2021

bors commented May 10, 2021

rust-timer commented May 10, 2021

rust-timer commented May 11, 2021

the8472 commented May 11, 2021

SkiFire13 commented May 11, 2021

JohnCSimon commented Nov 8, 2021

yaahc commented Nov 15, 2021

Mark-Simulacrum commented Dec 7, 2021

Mark-Simulacrum commented Dec 9, 2021

bors commented Dec 9, 2021

bors commented Dec 9, 2021

bors commented Dec 9, 2021

rust-timer commented Dec 9, 2021

the8472 commented Dec 9, 2021

RalfJung commented Dec 11, 2021

RalfJung Dec 11, 2021

Choose a reason for hiding this comment

the8472 Dec 11, 2021

Choose a reason for hiding this comment

RalfJung Dec 11, 2021

Choose a reason for hiding this comment

the8472 commented May 10, 2021 •

edited

Loading