Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize VecDeque::append #88717

Merged
merged 3 commits into from
Oct 15, 2021
Merged

Optimize VecDeque::append #88717

merged 3 commits into from
Oct 15, 2021

Conversation

tabokie
Copy link
Contributor

@tabokie tabokie commented Sep 7, 2021

Optimize VecDeque::append to do unsafe copy rather than iterating through each element.

On my Intel(R) Xeon(R) CPU E5-2630 v4 @ 2.20GHz, the benchmark shows 37% improvements:

Master:
custom-bench vec_deque_append 583164 ns/iter
custom-bench vec_deque_append 550040 ns/iter

Patched:
custom-bench vec_deque_append 349204 ns/iter
custom-bench vec_deque_append 368164 ns/iter

Additional notes on the context: this is the third attempt to implement a non-trivial version of VecDeque::append, the last two are reverted due to unsoundness or regression, see:

Both cases are covered by existing tests.

Signed-off-by: tabokie [email protected]

@rust-highfive
Copy link
Collaborator

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2021
@leonardo-m
Copy link

Is Miri silent?

@tabokie
Copy link
Contributor Author

tabokie commented Sep 7, 2021

Is Miri silent?

Sorry I'm new here, are you referring to running ./x.py test src/tools/miri? If so the results look good.

@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 8, 2021
@the8472
Copy link
Member

the8472 commented Sep 10, 2021

I think that only runs the dedicated tests in the miri test directory. You can also run all alloc tests under miri. https://github.com/rust-lang/miri-test-libstd

@tabokie
Copy link
Contributor Author

tabokie commented Sep 11, 2021

I think that only runs the dedicated tests in the miri test directory. You can also run all alloc tests under miri. https://github.com/rust-lang/miri-test-libstd

Passed.

@JohnCSimon 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 Sep 28, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Oct 3, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2021

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

@bors 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 Oct 3, 2021
@tabokie
Copy link
Contributor Author

tabokie commented Oct 8, 2021

Kindly ping @m-ou-se, do you know why bors hasn't attempted to merge this PR yet? 5 days of inactivity seems unusual. Is it because I appended one commit after your review?

@Mark-Simulacrum
Copy link
Member

@bors r=m-ou-se

Yes, pushing code will invalidate the approved state.

@bors
Copy link
Contributor

bors commented Oct 15, 2021

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

@matthiaskrgr
Copy link
Member

@bors rollup=never likely has some perf effects

@bors
Copy link
Contributor

bors commented Oct 15, 2021

⌛ Testing commit cd773c3 with merge af9b508...

@bors
Copy link
Contributor

bors commented Oct 15, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 15, 2021
@bors bors merged commit af9b508 into rust-lang:master Oct 15, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 15, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (af9b508): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.