Replace push loops with extend() where possible #52738
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Drive by comment, All these lovely clean up prs you have submitted... maybe we should suggest that clippy lint for them? So they can spread to the ecosystem? |
|
@Eh2406 clippy has an impressive set of working lints and some of them were a direct inspiration for some of my cleanups. I checked the ones that I haven't seen in clippy and it appears that they have already been proposed 😃: |
eddyb
reviewed
Jul 28, 2018
src/libsyntax/ast.rs
Outdated
| @@ -949,7 +949,7 @@ impl Expr { | |||
| ExprKind::Array(exprs) if exprs.len() == 1 => | |||
| exprs[0].to_ty().map(TyKind::Slice)?, | |||
| ExprKind::Tup(exprs) => { | |||
| let mut tys = Vec::new(); | |||
| let mut tys = Vec::with_capacity(exprs.len()); | |||
| for expr in exprs { | |||
| tys.push(expr.to_ty()?); | |||
There was a problem hiding this comment.
You can do this sort of thing by using .collect::<Result<Vec<_>, _>>() (for Result element types).
eddyb
reviewed
Jul 28, 2018
src/librustc/cfg/construct.rs
Outdated
| for &LoopScope { loop_id: id, .. } in self.loop_scopes.iter().rev() { | ||
| data.exiting_scopes.push(id); | ||
| } | ||
| data.exiting_scopes.extend( |
eddyb
reviewed
Jul 28, 2018
| for (_, obligation) in r_o.drain_filter(|(ro_body_id, _)| *ro_body_id == body_id) { | ||
| my_region_obligations.push(obligation); | ||
| } | ||
| my_region_obligations.extend( |
eddyb
reviewed
Jul 28, 2018
| } | ||
| signature_metadata.extend( | ||
| signature.inputs().iter().map(|argument_type| type_metadata(cx, argument_type, span)) | ||
| ); |
There was a problem hiding this comment.
Can be collect instead (using iter::once for the first element above).
eddyb
reviewed
Jul 28, 2018
| for n in glob_map.get(&id).unwrap() { | ||
| names.push(n.to_string()); | ||
| } | ||
| names.extend(glob_map.get(&id).unwrap().iter().map(|n| n.to_string())) |
eddyb
reviewed
Jul 28, 2018
src/libstd/sys/redox/process.rs
Outdated
| for arg in self.args.iter() { | ||
| args.push([arg.as_ptr() as usize, arg.len()]); | ||
| } | ||
| args.extend(self.args.iter().map(|arg| [arg.as_ptr() as usize, arg.len()])); |
There was a problem hiding this comment.
Can be collect instead (with iter::once).
eddyb
reviewed
Jul 28, 2018
| for declared_bound in ¶m.bounds { | ||
| bounds.push((*declared_bound).clone()); | ||
| } | ||
| bounds.extend(param.bounds.iter().cloned()); |
|
@eddyb great suggestions, applied. thanks! |
|
Rebased after 1bc49a9. |
eddyb
reviewed
Jul 29, 2018
| } | ||
| let mut r_o = self.region_obligations.borrow_mut(); | ||
| let my_region_obligations = r_o.drain_filter(|(ro_body_id, _)| *ro_body_id == body_id) | ||
| .map(|(_, obligation)| obligation); |
There was a problem hiding this comment.
Pretty sure you need to collect this, and not hold that borrow_mut for that long, because the loop calls methods on self which could try to borrow again self.region_obligations. cc @nikomatsakis
There was a problem hiding this comment.
I wanted to collect this, but the borrowed value didn't live long enough.
There was a problem hiding this comment.
What did you write? I'm guessing it's just some scope thing.
There was a problem hiding this comment.
In general, let v = { let mut x = y.borrow_mut(); x.foo().collect() }; doesn't work, you need another variable inside the block, e.g. let v = { let mut x = y.borrow_mut(); let v = x.foo().collect(); v };
There was a problem hiding this comment.
Ah yeah, that should work; I'll fix this shortly.
eddyb
reviewed
Jul 29, 2018
src/libsyntax/ast.rs
Outdated
| @@ -500,7 +500,7 @@ impl Pat { | |||
| PatKind::Slice(pats, None, _) if pats.len() == 1 => | |||
| pats[0].to_ty().map(TyKind::Slice)?, | |||
| PatKind::Tuple(pats, None) => { | |||
| let mut tys = Vec::new(); | |||
| let mut tys = Vec::with_capacity(pats.len()); | |||
| for pat in pats { | |||
| tys.push(pat.to_ty()?); | |||
There was a problem hiding this comment.
Pretty sure this can also use collect (it's a lot like the code below).
eddyb
approved these changes
Jul 29, 2018
|
@bors r+ |
|
📌 Commit 6071fb762a3add82a54307fbe79df83c5c9bdb85 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 6071fb762a3add82a54307fbe79df83c5c9bdb85 with merge e7f363a2f3b01512e6ca1cdd569db506711f38a5... |
|
💔 Test failed - status-travis |
bors
removed
the
S-waiting-on-bors
bors
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ah yes, Redox isn't |
|
@eddyb can you ask bors to retry? |
|
@bors r+ |
|
📌 Commit 59c8a27 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 29, 2018
Replace push loops with extend() where possible Or set the vector capacity where I couldn't do it. According to my [simple benchmark](https://gist.github.com/ljedrz/568e97621b749849684c1da71c27dceb) `extend`ing a vector can be over **10 times** faster than `push`ing to it in a loop: 10 elements (6.1 times faster): ``` test bench_extension ... bench: 75 ns/iter (+/- 23) test bench_push_loop ... bench: 458 ns/iter (+/- 142) ``` 100 elements (11.12 times faster): ``` test bench_extension ... bench: 87 ns/iter (+/- 26) test bench_push_loop ... bench: 968 ns/iter (+/- 3,528) ``` 1000 elements (11.04 times faster): ``` test bench_extension ... bench: 311 ns/iter (+/- 9) test bench_push_loop ... bench: 3,436 ns/iter (+/- 233) ``` Seems like a good idea to use `extend` as much as possible.
|
☀️ Test successful - status-appveyor, status-travis |
Mark-Simulacrum
added a commit
to Mark-Simulacrum/rust
that referenced
this pull request
Aug 1, 2018
…-Simulacrum Use Vec::extend in SmallVec::extend when applicable As calculated in rust-lang#52738, `Vec::extend` is much faster than `push`ing to it in a loop. We can take advantage of this method in `SmallVec` too - at least in cases when its underlying object is an `AccumulateVec::Heap`. ~~This approach also accidentally improves the `push` loop of the `AccumulateVec::Array` variant, because it doesn't utilize `SmallVec::push` which performs `self.reserve(1)` with every iteration; this is unnecessary, because we're already reserving the whole space we will be needing by performing `self.reserve(iter.size_hint().0)` at the beginning.~~
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this pull request
Aug 1, 2018
…-Simulacrum Use Vec::extend in SmallVec::extend when applicable As calculated in rust-lang#52738, `Vec::extend` is much faster than `push`ing to it in a loop. We can take advantage of this method in `SmallVec` too - at least in cases when its underlying object is an `AccumulateVec::Heap`. ~~This approach also accidentally improves the `push` loop of the `AccumulateVec::Array` variant, because it doesn't utilize `SmallVec::push` which performs `self.reserve(1)` with every iteration; this is unnecessary, because we're already reserving the whole space we will be needing by performing `self.reserve(iter.size_hint().0)` at the beginning.~~
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Aug 10, 2018
Don't collect() when size_hint is useless This adjusts PRs rust-lang#52738 and rust-lang#52697 by falling back to calculating capacity and extending or pushing in a loop where `collect()` can't be trusted to calculate the right capacity. It is a performance win.
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Aug 12, 2018
Don't collect() when size_hint is useless This adjusts PRs rust-lang#52738 and rust-lang#52697 by falling back to calculating capacity and extending or pushing in a loop where `collect()` can't be trusted to calculate the right capacity. It is a performance win.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Or set the vector capacity where I couldn't do it.
According to my simple benchmark
extending a vector can be over 10 times faster thanpushing to it in a loop:10 elements (6.1 times faster):
100 elements (11.12 times faster):
1000 elements (11.04 times faster):
Seems like a good idea to use
extendas much as possible.