Uh oh!
There was an error while loading. Please reload this page.
Optimise Iterator::{max, max_by, min, min_by}. - #24180
Merged
Merged
Conversation
rust-highfive
commented
Apr 8, 2015
Contributor
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
Contributor
There was a problem hiding this comment.
Is there some reason the comparison function takes 4 arguments, when each client only wants 2 of them, instead of just making it take the projections and having max() / min() use |x| x for the projection function?
ContributorAuthor
There was a problem hiding this comment.
I agree it would be nice but the lifetimes don't work out, B is disconnected from &I::Item. I think the fact we move sel and x around internally means that this approach can't work. :(
alexcrichton
commented
Apr 9, 2015
Member
r=me with nits addressed, nice wins! |
The main change in this patch is removing the use of `Option` inside the inner loops of those functions to avoid comparisons where one branch will only trigger on the first pass through the loop. The included benchmarks go from: test bench_max ... bench: 372 ns/iter (+/- 118) test bench_max_by ... bench: 428 ns/iter (+/- 33) test bench_max_by2 ... bench: 7128 ns/iter (+/- 326) to: test bench_max ... bench: 317 ns/iter (+/- 64) test bench_max_by ... bench: 356 ns/iter (+/- 270) test bench_max_by2 ... bench: 1387 ns/iter (+/- 183) Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
huonw
commented
Apr 10, 2015
ContributorAuthor
@bors r=alexcrichton |
bors
commented
Apr 10, 2015
Collaborator
📌 Commit c2258d6 has been approved by |
bors
commented
Apr 10, 2015
Collaborator
⌛ Testing commit c2258d6 with merge f4aa02e... |
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Apr 10, 2015
The main change in this patch is removing the use of `Option` inside the inner loops of those functions to avoid comparisons where one branch will only trigger on the first pass through the loop. The included benchmarks go from: test bench_max ... bench: 372 ns/iter (+/- 118) test bench_max_by ... bench: 428 ns/iter (+/- 33) test bench_max_by2 ... bench: 7128 ns/iter (+/- 326) to: test bench_max ... bench: 317 ns/iter (+/- 64) test bench_max_by ... bench: 356 ns/iter (+/- 270) test bench_max_by2 ... bench: 1387 ns/iter (+/- 183) Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
bors
commented
Apr 10, 2015
Collaborator
⛄ The build was interrupted to prioritize another pull request. |
bors
commented
Apr 10, 2015
Collaborator
⌛ Testing commit c2258d6 with merge 1302028... |
Manishearth added a commit
to Manishearth/rust
that referenced
this pull request
Apr 10, 2015
The main change in this patch is removing the use of `Option` inside the inner loops of those functions to avoid comparisons where one branch will only trigger on the first pass through the loop. The included benchmarks go from: test bench_max ... bench: 372 ns/iter (+/- 118) test bench_max_by ... bench: 428 ns/iter (+/- 33) test bench_max_by2 ... bench: 7128 ns/iter (+/- 326) to: test bench_max ... bench: 317 ns/iter (+/- 64) test bench_max_by ... bench: 356 ns/iter (+/- 270) test bench_max_by2 ... bench: 1387 ns/iter (+/- 183) Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
bors
commented
Apr 10, 2015
Collaborator
⛄ The build was interrupted to prioritize another pull request. |
bors
commented
Apr 10, 2015
Collaborator
bors added a commit
that referenced
this pull request
Apr 10, 2015
The main change in this patch is removing the use of `Option` inside the inner loops of those functions to avoid comparisons where one branch will only trigger on the first pass through the loop. The included benchmarks go from: test bench_max ... bench: 372 ns/iter (+/- 118) test bench_max_by ... bench: 428 ns/iter (+/- 33) test bench_max_by2 ... bench: 7128 ns/iter (+/- 326) to: test bench_max ... bench: 317 ns/iter (+/- 64) test bench_max_by ... bench: 356 ns/iter (+/- 270) test bench_max_by2 ... bench: 1387 ns/iter (+/- 183) Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
bors
commented
Apr 10, 2015
Collaborator
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
The main change in this patch is removing the use of
Optioninside theinner loops of those functions to avoid comparisons where one branch
will only trigger on the first pass through the loop.
The included benchmarks go from:
to:
Problem noticed in http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/