Skip to content

Match VecDeque::extend to Vec::extend_desugared - #66341

Merged
bors merged 1 commit into
rust-lang:masterfrom
crgl:vec-deque-extend
Dec 13, 2019
Merged

Match VecDeque::extend to Vec::extend_desugared#66341
bors merged 1 commit into
rust-lang:masterfrom
crgl:vec-deque-extend

Conversation

@crgl

@crglcrgl commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

Currently, VecDeque::extenddoes not reserve at all. This implementation still runs a check every iteration of the loop, but should reallocate at most once for the common cases where the size_hint lower bound is exact. Further optimizations in the future could improve this for some common cases, but given the complexity of the Vec::extend implementation it's not immediately clear that this would be worthwhile.

@rust-highfive

rust-highfive commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2019
hellow554
hellow554 reviewed Nov 13, 2019
View reviewed changes
Comment threadsrc/liballoc/collections/vec_deque.rs Outdated
@joelpalmer

joelpalmer commented Nov 18, 2019

Copy link
Copy Markdown

Ping from Triage: Any update? @hellow554@Kimundi?

@Dylan-DPC-zz

Dylan-DPC-zz commented Nov 22, 2019

Copy link
Copy Markdown

r? @SimonSapin

@wirelessringo

wirelessringo commented Nov 29, 2019

Copy link
Copy Markdown

Ping from triage

@SimonSapin any updates on this? Thanks.

@SimonSapin

SimonSapin commented Nov 29, 2019

Copy link
Copy Markdown
Contributor

Sorry. I’m not familiar enough with internals of VecDeque to know off-hand if this direct manipulation of self.head is correct, and without benchmarks to show whether this is actually an improvement I don’t know if it’s worth spending the time to read up all of VecDeque to learn how it works. Also, I’ve removed myself from the review auto-assignment because of limited bandwidth. Unless you’re confident that I have experience on something that another reviewer won’t, please consider asking other people for reviews.

r? @Dylan-DPC

@Dylan-DPC-zz

Dylan-DPC-zz commented Dec 5, 2019

Copy link
Copy Markdown

r? @dtolnay

@Dylan-DPC-zz

Dylan-DPC-zz commented Dec 12, 2019

Copy link
Copy Markdown

r? @Amanieu

@rust-highfiverust-highfive assigned Amanieu and unassigned dtolnayDec 12, 2019
@Amanieu

Amanieu commented Dec 12, 2019

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Dec 12, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 164d1a2 has been approved by Amanieu

@borsbors 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 12, 2019
@bors

bors commented Dec 13, 2019

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 164d1a2 with merge 6c6235d30d605fa38b55d059a8c57b3e1149885b...

Centril added a commit to Centril/rust that referenced this pull request Dec 13, 2019
@Centril
Rollup merge ofrust-lang#66341- crgl:vec-deque-extend, r=Amanieu
2ca7b7e
Match `VecDeque::extend` to `Vec::extend_desugared`
Currently, `VecDeque::extend` [does not reserve at all](rust-lang#65069 (comment)). This implementation still runs a check every iteration of the loop, but should reallocate at most once for the common cases where the `size_hint` lower bound is exact. Further optimizations in the future could improve this for some common cases, but given the complexity of the `Vec::extend` implementation it's not immediately clear that this would be worthwhile.
@CentrilCentril mentioned this pull request Dec 13, 2019
Merged
@Centril

Centril commented Dec 13, 2019

Copy link
Copy Markdown
Contributor

@bors retry rolled up.

bors added a commit that referenced this pull request Dec 13, 2019
@bors
Auto merge of#67271- Centril:rollup-i71iqkv, r=Centril
cf7e019
Rollup of 6 pull requests
Successful merges:
- #66341 (Match `VecDeque::extend` to `Vec::extend_desugared`)
- #67243 (LinkedList: drop remaining items when drop panics)
- #67247 (Don't suggest wrong snippet in closure)
- #67250 (Remove the `DelimSpan` from `NamedMatch::MatchedSeq`.)
- #67251 (Require `allow_internal_unstable` for stable min_const_fn using unsta…)
- #67269 (parser: recover on `&'lifetime mut? $pat`.)
Failed merges:
r? @ghost
@bors

bors commented Dec 13, 2019

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 164d1a2 with merge cf7e019...

@bors
bors merged commit 164d1a2 into rust-lang:masterDec 13, 2019
@crgl
crgl deleted the vec-deque-extend branch December 13, 2019 07:34
@dtolnaydtolnay mentioned this pull request Dec 13, 2019
Merged
Lonami
Lonami reviewed Dec 19, 2019
View reviewed changes
Comment threadsrc/liballoc/collections/vec_deque.rs
// }
let mut iter = iter.into_iter();
while let Some(element) = iter.next() {
if self.len() == self.capacity() {

@LonamiLonamiDec 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This implementation still runs a check every iteration of the loop

Wouldn't it be possible to move this out of the loop?

let capacity = self.capacity() - self.len();let(lower, _) = iter.size_hint();if capacity < lower {self.reserve(...);}

@crglcrglJan 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You could do it for trusted length iterators specifically, but otherwise because it's a lower bound you have to check every time. At some point it could be good to specialize, though

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@hellow554hellow554hellow554 left review comments
@LonamiLonamiLonami left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@AmanieuAmanieu

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

13 participants

@crgl@rust-highfive@joelpalmer@Dylan-DPC-zz@wirelessringo@SimonSapin@Amanieu@bors@Centril@hellow554@Lonami@dtolnay@Kimundi