Use MaybeUninit in VecDeque to remove the undefined behavior of slice #94472

JmPotato · 2022-03-01T05:27:22Z

Signed-off-by: JmPotato ghzpotato@gmail.com

Ref #74189. Adjust the code to follow the doc.rust-lang.org/reference/behavior-considered-undefined.html.

Change the return type of buffer_as_slice from &[T] to &[MaybeUninit<T>].
Add some corresponding safety comments.

Benchmark results:

master 8d6f527

test collections::vec_deque::tests::bench_pop_back_100       ... bench:          47 ns/iter (+/- 1)
test collections::vec_deque::tests::bench_pop_front_100      ... bench:          50 ns/iter (+/- 4)
test collections::vec_deque::tests::bench_push_back_100      ... bench:          69 ns/iter (+/- 10)
test collections::vec_deque::tests::bench_push_front_100     ... bench:          72 ns/iter (+/- 6)
test collections::vec_deque::tests::bench_retain_half_10000  ... bench:     145,891 ns/iter (+/- 7,975)
test collections::vec_deque::tests::bench_retain_odd_10000   ... bench:     141,647 ns/iter (+/- 3,711)
test collections::vec_deque::tests::bench_retain_whole_10000 ... bench:     120,132 ns/iter (+/- 4,078)

This PR

test collections::vec_deque::tests::bench_pop_back_100       ... bench:          48 ns/iter (+/- 2)
test collections::vec_deque::tests::bench_pop_front_100      ... bench:          51 ns/iter (+/- 3)
test collections::vec_deque::tests::bench_push_back_100      ... bench:          73 ns/iter (+/- 2)
test collections::vec_deque::tests::bench_push_front_100     ... bench:          73 ns/iter (+/- 2)
test collections::vec_deque::tests::bench_retain_half_10000  ... bench:     131,796 ns/iter (+/- 5,440)
test collections::vec_deque::tests::bench_retain_odd_10000   ... bench:     137,563 ns/iter (+/- 3,349)
test collections::vec_deque::tests::bench_retain_whole_10000 ... bench:     128,815 ns/iter (+/- 3,289)

rust-highfive · 2022-03-01T05:27:25Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

JmPotato · 2022-03-01T05:28:29Z

r? @RalfJung

RalfJung · 2022-03-01T14:21:42Z

Sorry, I have a too large backlog of Rust things to do to review this, and have no familiarity with this code.

r? rust-lang/libs

JmPotato · 2022-03-03T04:34:05Z

@rustbot label +T-libs

JmPotato · 2022-03-07T04:00:12Z

Friendly ping~ @m-ou-se

JmPotato · 2022-03-09T03:15:15Z

Since I haven't received any feedback for a while, I want to try to re-invite a member from the list to help me review, thanks for understanding.

r? @dtolnay

m-ou-se

Nice, thank you for working on this! I left one small comment, but otherwise this looks good to me.

Since I haven't received any feedback for a while, [..]

Unfortunately we're all quite busy and there's a lot of PRs to review, so it's not uncommon for a PR to sit still for a week or two. Unless it's a very critical issue, two weeks is probably a good time to send a ping or reassign it.

library/alloc/src/collections/vec_deque/iter.rs

JmPotato · 2022-03-09T13:26:55Z

Nice, thank you for working on this! I left one small comment, but otherwise this looks good to me.

Since I haven't received any feedback for a while, [..]

Unfortunately we're all quite busy and there's a lot of PRs to review, so it's not uncommon for a PR to sit still for a week or two. Unless it's a very critical issue, two weeks is probably a good time to send a ping or reassign it.

@m-ou-se Thanks for the explanation! It seems that I am too eager, xD. I have addressed the comment. PTAL.

m-ou-se · 2022-03-09T13:53:57Z

Thanks!

@bors r+ rollup

bors · 2022-03-09T13:53:58Z

📌 Commit e54d4ab has been approved by m-ou-se

…ue, r=m-ou-se Use MaybeUninit in VecDeque to remove the undefined behavior of slice Signed-off-by: JmPotato <ghzpotato@gmail.com> Ref rust-lang#74189. Adjust the code to follow the [doc.rust-lang.org/reference/behavior-considered-undefined.html](https://doc.rust-lang.org/reference/behavior-considered-undefined.html). * Change the return type of `buffer_as_slice` from `&[T]` to `&[MaybeUninit<T>]`. * Add some corresponding safety comments. Benchmark results: master 8d6f527 ```rust test collections::vec_deque::tests::bench_pop_back_100 ... bench: 47 ns/iter (+/- 1) test collections::vec_deque::tests::bench_pop_front_100 ... bench: 50 ns/iter (+/- 4) test collections::vec_deque::tests::bench_push_back_100 ... bench: 69 ns/iter (+/- 10) test collections::vec_deque::tests::bench_push_front_100 ... bench: 72 ns/iter (+/- 6) test collections::vec_deque::tests::bench_retain_half_10000 ... bench: 145,891 ns/iter (+/- 7,975) test collections::vec_deque::tests::bench_retain_odd_10000 ... bench: 141,647 ns/iter (+/- 3,711) test collections::vec_deque::tests::bench_retain_whole_10000 ... bench: 120,132 ns/iter (+/- 4,078) ``` This PR ```rust test collections::vec_deque::tests::bench_pop_back_100 ... bench: 48 ns/iter (+/- 2) test collections::vec_deque::tests::bench_pop_front_100 ... bench: 51 ns/iter (+/- 3) test collections::vec_deque::tests::bench_push_back_100 ... bench: 73 ns/iter (+/- 2) test collections::vec_deque::tests::bench_push_front_100 ... bench: 73 ns/iter (+/- 2) test collections::vec_deque::tests::bench_retain_half_10000 ... bench: 131,796 ns/iter (+/- 5,440) test collections::vec_deque::tests::bench_retain_odd_10000 ... bench: 137,563 ns/iter (+/- 3,349) test collections::vec_deque::tests::bench_retain_whole_10000 ... bench: 128,815 ns/iter (+/- 3,289) ```

matthiaskrgr · 2022-03-09T22:13:14Z

@bors r-
failed in a rollup it seems:
#94784 (comment)
@bors rollup=never (perf change)

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato · 2022-03-10T06:14:53Z

@bors r- failed in a rollup it seems: #94784 (comment) @bors rollup=never (perf change)

@matthiaskrgr @m-ou-se Hi, I updated the Debug implementation of VecDeque to keep compatible with the previous behavior. This should fix the failed test, PTAL again, thx!

JmPotato · 2022-03-10T09:46:34Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

m-ou-se · 2022-03-10T10:26:01Z

@bors r+

bors · 2022-03-10T10:26:02Z

📌 Commit 2f18fa8 has been approved by m-ou-se

JmPotato · 2022-03-11T10:01:46Z

@bors try

bors · 2022-03-11T10:01:48Z

@JmPotato: 🔑 Insufficient privileges: not in try users

bors · 2022-03-11T19:23:58Z

⌛ Testing commit 2f18fa8 with merge 335ffbf...

bors · 2022-03-11T21:44:00Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 335ffbf to master...

rust-timer · 2022-03-12T00:08:34Z

Finished benchmarking commit (335ffbf): comparison url.

Summary: This benchmark run shows 1 relevant improvement 🎉 but 3 relevant regressions 😿 to instruction counts.

Arithmetic mean of relevant regressions: 0.9%
Arithmetic mean of all relevant changes: -2.0%
Largest improvement in instruction counts: -10.7% on incr-patched: println builds of tokio-webpush-simple opt
Largest regression in instruction counts: 1.1% on full builds of tokio-webpush-simple opt

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

rylev · 2022-03-15T13:23:12Z

The micro benchmarks indicate that this is largely a performance wash. Most benchmarks don't seem to show statistical difference and those that do are a mix of small regressions and improvements. It is surprising to see one very large improvement though. Given this is a correctness fix though and overall the perf changes leaned towards improvement, I think we can live with this.

@rustbot label: +perf-regression-triaged

rust-highfive assigned kennytm Mar 1, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 1, 2022

rust-highfive assigned RalfJung and unassigned kennytm Mar 1, 2022

rust-highfive assigned m-ou-se and unassigned RalfJung Mar 1, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 3, 2022

rust-highfive assigned dtolnay and unassigned m-ou-se Mar 9, 2022

m-ou-se reviewed Mar 9, 2022

View reviewed changes

library/alloc/src/collections/vec_deque/iter.rs Outdated Show resolved Hide resolved

library/alloc/src/collections/vec_deque/iter.rs Outdated Show resolved Hide resolved

m-ou-se assigned m-ou-se and unassigned dtolnay Mar 9, 2022

m-ou-se 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 Mar 9, 2022

JmPotato force-pushed the use_maybeuninit_for_vecdeque branch from bf5a367 to e54d4ab Compare March 9, 2022 13:23

JmPotato requested a review from m-ou-se March 9, 2022 13:39

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 9, 2022

matthiaskrgr mentioned this pull request Mar 9, 2022

Rollup of 8 pull requests #94771

Closed

matthiaskrgr mentioned this pull request Mar 9, 2022

Rollup of 7 pull requests #94774

Closed

Dylan-DPC mentioned this pull request Mar 9, 2022

Rollup of 16 pull requests #94784

Closed

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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 9, 2022

Use MaybeUninit in VecDeque to remove the undefined behavior of slice

2f18fa8

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato force-pushed the use_maybeuninit_for_vecdeque branch from e54d4ab to 2f18fa8 Compare March 10, 2022 06:14

rustbot 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 Mar 10, 2022

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 Mar 10, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 11, 2022

bors merged commit 335ffbf into rust-lang:master Mar 11, 2022

rustbot added this to the 1.61.0 milestone Mar 11, 2022

bors mentioned this pull request Mar 11, 2022

refactor: VecDeques Iter fields to private #89685

Merged

rustbot added the perf-regression Performance regression. label Mar 12, 2022

JmPotato deleted the use_maybeuninit_for_vecdeque branch March 12, 2022 01:05

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 15, 2022

Use MaybeUninit in VecDeque to remove the undefined behavior of slice #94472

Use MaybeUninit in VecDeque to remove the undefined behavior of slice #94472

Uh oh!

Conversation

JmPotato commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 1, 2022

Uh oh!

JmPotato commented Mar 1, 2022

Uh oh!

RalfJung commented Mar 1, 2022

Uh oh!

JmPotato commented Mar 3, 2022

Uh oh!

JmPotato commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JmPotato commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-ou-se left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JmPotato commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-ou-se commented Mar 9, 2022

Uh oh!

bors commented Mar 9, 2022

Uh oh!

matthiaskrgr commented Mar 9, 2022

Uh oh!

JmPotato commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JmPotato commented Mar 10, 2022

Uh oh!

m-ou-se commented Mar 10, 2022

Uh oh!

bors commented Mar 10, 2022

Uh oh!

JmPotato commented Mar 11, 2022

Uh oh!

bors commented Mar 11, 2022

Uh oh!

bors commented Mar 11, 2022

Uh oh!

bors commented Mar 11, 2022

Uh oh!

rust-timer commented Mar 12, 2022

Uh oh!

rylev commented Mar 15, 2022

Uh oh!

Uh oh!

JmPotato commented Mar 1, 2022 •

edited

Loading

JmPotato commented Mar 7, 2022 •

edited

Loading

JmPotato commented Mar 9, 2022 •

edited

Loading

JmPotato commented Mar 9, 2022 •

edited

Loading

JmPotato commented Mar 10, 2022 •

edited

Loading