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.

Sign up for GitHub

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

Jump to bottom

Add VecDeque::extend from vec::IntoIter and slice::Iter specializations #95904

Merged
merged 2 commits into from
Apr 28, 2022
Merged

Add VecDeque::extend from vec::IntoIter and slice::Iter specializations #95904

merged 2 commits into from
Apr 28, 2022

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Apr 10, 2022 •
edited
Loading

Inspired from the Vec SpecExtend implementation, but without the specialization for TrustedLen which I'll look into in the future.

Should help #95632 and KillingSpark/zstd-rs#17

Benchmarks

Before

test vec_deque::bench_extend_bytes    ... bench:         862 ns/iter (+/- 10)
test vec_deque::bench_extend_vec      ... bench:         883 ns/iter (+/- 19)

After

test vec_deque::bench_extend_bytes    ... bench:           8 ns/iter (+/- 0)
test vec_deque::bench_extend_vec      ... bench:          24 ns/iter (+/- 1)

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2022
@paolobarbolini paolobarbolini mentioned this pull request Apr 10, 2022
Merged
@paolobarbolini paolobarbolini mentioned this pull request Apr 11, 2022
Closed
the8472
the8472 reviewed Apr 11, 2022
View reviewed changes
library/alloc/src/collections/vec_deque/mod.rs Outdated Show resolved Hide resolved
library/alloc/src/vec/into_iter.rs Outdated Show resolved Hide resolved
@paolobarbolini
Copy link
Contributor Author

r? rust-lang/libs

@rust-highfive rust-highfive assigned yaahc and unassigned joshtriplett Apr 27, 2022
@the8472 the8472 assigned the8472 and unassigned yaahc Apr 27, 2022
the8472
the8472 reviewed Apr 27, 2022
View reviewed changes
Copy link
Member

@the8472 the8472 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you added a benchmark, can you post results? Or show a difference in assembly output? Also, that benchmark only tests the borrowed case, not the owning one.

library/alloc/src/collections/vec_deque/mod.rs Outdated Show resolved Hide resolved
@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 27, 2022
@paolobarbolini
Copy link
Contributor Author

I see that you added a benchmark, can you post results?

I've updated the PR description with the results of the benchmarks

Also, that benchmark only tests the borrowed case, not the owning one.

I've added the vec::IntoIter benchmark

@the8472
Copy link
Member

the8472 commented Apr 28, 2022

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 28, 2022

📌 Commit c126f7f has been approved by the8472

@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 Apr 28, 2022
@bors
Copy link
Contributor

bors commented Apr 28, 2022

⌛ Testing commit c126f7f with merge 3bfeffd...

@bors
Copy link
Contributor

bors commented Apr 28, 2022

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 3bfeffd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 28, 2022
@bors bors merged commit 3bfeffd into rust-lang:master Apr 28, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 28, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3bfeffd): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 1 0 0 0 1
mean2 0.4% N/A N/A N/A 0.4%
max 0.4% N/A N/A N/A 0.4%

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

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@paolobarbolini paolobarbolini mentioned this pull request Jun 9, 2022
Merged
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 9, 2022
@compiler-errors
Rollup merge of rust-lang#97922 - paolobarbolini:no-vecdeque-extra-re…
09d72f5 
…serve, r=the8472

Remove redundant calls to reserve in impl Write for VecDeque

Removes the reserve calls made redundant by rust-lang#95904 (as discussed in rust-lang#95632 (comment))
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 10, 2022
@Dylan-DPC
Rollup merge of rust-lang#97922 - paolobarbolini:no-vecdeque-extra-re…
eca7229 
…serve, r=the8472

Remove redundant calls to reserve in impl Write for VecDeque

Removes the reserve calls made redundant by rust-lang#95904 (as discussed in rust-lang#95632 (comment))
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 10, 2022
@JohnTitor
Rollup merge of rust-lang#97922 - paolobarbolini:no-vecdeque-extra-re…
3e5ddb7 
…serve, r=the8472

Remove redundant calls to reserve in impl Write for VecDeque

Removes the reserve calls made redundant by rust-lang#95904 (as discussed in rust-lang#95632 (comment))
@paolobarbolini paolobarbolini mentioned this pull request Jun 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 18, 2022
@bors
Auto merge of rust-lang#98004 - paolobarbolini:vecdeque-extend-truste…
2cec687 
…dlen, r=the8472

Add VecDeque::extend from TrustedLen specialization

Continuation of rust-lang#95904

Inspired by how [`VecDeque::copy_slice` works](https://github.com/rust-lang/rust/blob/c08b235a5ce10167632bb0fddcd0c5d67f2d42e3/library/alloc/src/collections/vec_deque/mod.rs#L437-L454).

## Benchmarks

Before

```
test vec_deque::bench_extend_chained_bytes      ... bench:       1,026 ns/iter (+/- 17)
test vec_deque::bench_extend_chained_trustedlen ... bench:       1,024 ns/iter (+/- 40)
test vec_deque::bench_extend_trustedlen         ... bench:         637 ns/iter (+/- 693)
```

After

```
test vec_deque::bench_extend_chained_bytes      ... bench:         828 ns/iter (+/- 24)
test vec_deque::bench_extend_chained_trustedlen ... bench:          25 ns/iter (+/- 1)
test vec_deque::bench_extend_trustedlen         ... bench:          21 ns/iter (+/- 0)
```

## Why do it this way

https://rust.godbolt.org/z/15qY1fMYh

The Compiler Explorer example shows how "just" removing the capacity check, like the [`Vec` `TrustedLen` specialization](https://github.com/rust-lang/rust/blob/c08b235a5ce10167632bb0fddcd0c5d67f2d42e3/library/alloc/src/vec/spec_extend.rs#L22-L58) does, wouldn't have been enough for `VecDeque`. `wrap_add` would still have greatly limited what LLVM could do while optimizing.

---

r? `@the8472`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the8472 the8472 the8472 left review comments

Assignees

@the8472 the8472

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
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
8 participants
@paolobarbolini @rust-highfive @the8472 @bors @rust-timer @joshtriplett @yaahc @rustbot