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

Use for_each to extend collections #59740

Merged
merged 1 commit into from Apr 7, 2019

Conversation

Projects
None yet
8 participants
@cuviper
Copy link
Member

commented Apr 5, 2019

This updates the Extend implementations to use for_each for many
collections: BinaryHeap, BTreeMap, BTreeSet, LinkedList, Path,
TokenStream, VecDeque, and Wtf8Buf.

Folding with for_each enables better performance than a for-loop for
some iterators, especially if they can just forward to internal
iterators, like Chain and FlatMap do.

Use for_each to extend collections
This updates the `Extend` implementations to use `for_each` for many
collections: `BinaryHeap`, `BTreeMap`, `BTreeSet`, `LinkedList`, `Path`,
`TokenStream`, `VecDeque`, and `Wtf8Buf`.

Folding with `for_each` enables better performance than a `for`-loop for
some iterators, especially if they can just forward to internal
iterators, like `Chain` and `FlatMap` do.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

r? @TimNN

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

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

For reference:

  • Vec was updated in #54761
  • String was updated in #56548
  • HashMap is being replaced by hashbrown in #58623
    • The same treatment is offered in Amanieu/hashbrown#58
@Centril

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@rust-highfive rust-highfive assigned scottmcm and unassigned TimNN Apr 5, 2019

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

To get at least a sense that this isn't obviously bad (though I don't expect it to be), let's @bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

⌛️ Trying commit 0730a01 with merge eab4756...

bors added a commit that referenced this pull request Apr 6, 2019

Auto merge of #59740 - cuviper:folded-extend, r=<try>
Use for_each to extend collections

This updates the `Extend` implementations to use `for_each` for many
collections: `BinaryHeap`, `BTreeMap`, `BTreeSet`, `LinkedList`, `Path`,
`TokenStream`, `VecDeque`, and `Wtf8Buf`.

Folding with `for_each` enables better performance than a `for`-loop for
some iterators, especially if they can just forward to internal
iterators, like `Chain` and `FlatMap` do.
@scottmcm

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

r=me when perf is happy

@scottmcm

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Actually, you might check for while let loops too:

while let Some(element) = iterator.next() {

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

I see, the previous Vec improvement was only for I: TrustedLen. I'll look for more...

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

@scottmcm that one is difficult, because it wants to check size_hint() every time it needs to grow, which we can't do while folding...

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

☀️ Try build successful - checks-travis
Build commit: eab4756

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Apr 6, 2019

Success: Queued eab4756 with parent acd8dd6, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Apr 6, 2019

Finished benchmarking try commit eab4756

@scottmcm

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@cuviper Oops! You're right. And perf looks happy, so

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

📌 Commit 0730a01 has been approved by scottmcm

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Apr 6, 2019

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #59724
@bors

This comment was marked as resolved.

Copy link
Contributor

commented Apr 6, 2019

📌 Commit 0730a01 has been approved by scottmcm

Centril added a commit to Centril/rust that referenced this pull request Apr 6, 2019

Rollup merge of rust-lang#59740 - cuviper:folded-extend, r=scottmcm
Use for_each to extend collections

This updates the `Extend` implementations to use `for_each` for many
collections: `BinaryHeap`, `BTreeMap`, `BTreeSet`, `LinkedList`, `Path`,
`TokenStream`, `VecDeque`, and `Wtf8Buf`.

Folding with `for_each` enables better performance than a `for`-loop for
some iterators, especially if they can just forward to internal
iterators, like `Chain` and `FlatMap` do.

@Centril Centril referenced this pull request Apr 6, 2019

Closed

Rollup of 5 pull requests #59758

Centril added a commit to Centril/rust that referenced this pull request Apr 6, 2019

Rollup merge of rust-lang#59740 - cuviper:folded-extend, r=scottmcm
Use for_each to extend collections

This updates the `Extend` implementations to use `for_each` for many
collections: `BinaryHeap`, `BTreeMap`, `BTreeSet`, `LinkedList`, `Path`,
`TokenStream`, `VecDeque`, and `Wtf8Buf`.

Folding with `for_each` enables better performance than a `for`-loop for
some iterators, especially if they can just forward to internal
iterators, like `Chain` and `FlatMap` do.

@Centril Centril referenced this pull request Apr 6, 2019

Merged

Rollup of 5 pull requests #59760

bors added a commit that referenced this pull request Apr 6, 2019

Auto merge of #59760 - Centril:rollup-4b9x7ue, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #59738 (Move match_path from DefId to lint::LateContext)
 - #59740 (Use for_each to extend collections)
 - #59751 (Tiny docs fix)
 - #59754 (Update books)
 - #59755 (Update miri)

Failed merges:

r? @ghost

@bors bors merged commit 0730a01 into rust-lang:master Apr 7, 2019

1 check passed

homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.