Specialize PartialOrd<A> for [A] where A: Ord
#39642
Merged
Conversation
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
This way we can call `cmp` instead of `partial_cmp` in the loop, removing some burden of optimizing `Option`s away from the compiler. PR #39538 introduced a regression where sorting slices suddenly became slower, since `slice1.lt(slice2)` was much slower than `slice1.cmp(slice2) == Less`. This problem is now fixed. To verify, I benchmarked this simple program: ```rust fn main() { let mut v = (0..2_000_000).map(|x| x * x * x * 18913515181).map(|x| vec![x, x ^ 3137831591]).collect::<Vec<_>>(); v.sort(); } ``` Before this PR, it would take 0.95 sec, and now it takes 0.58 sec. I also tried changing the `is_less` lambda to use `cmp` and `partial_cmp`. Now all three versions (`lt`, `cmp`, `partial_cmp`) are equally performant for sorting slices - all of them take 0.58 sec on the benchmark.
ollie27
reviewed
Feb 8, 2017
src/libcore/slice.rs
Outdated
| self.len().partial_cmp(&other.len()) | ||
| } | ||
| } | ||
|
|
||
| impl SlicePartialOrd<u8> for [u8] { |
There was a problem hiding this comment.
Could this impl not be extended to all A: Ord so it would simply use Some(SliceOrd::compare(self, other))?
There was a problem hiding this comment.
That's a good suggestion, thanks! I've updated the code.
alexcrichton
reviewed
Feb 8, 2017
src/libcore/slice.rs
Outdated
| Some(SliceOrd::compare(self, other)) | ||
| } | ||
| } | ||
|
|
||
| impl SlicePartialOrd<u8> for [u8] { |
There was a problem hiding this comment.
I think this specialization could be removed now, right?
alexcrichton
added
the
T-libs-api
|
@bors: r+ |
|
📌 Commit a344c12 has been approved by |
bors
added a commit
that referenced
this pull request
Feb 11, 2017
…ichton Specialize `PartialOrd<A> for [A] where A: Ord` This way we can call `cmp` instead of `partial_cmp` in the loop, removing some burden of optimizing `Option`s away from the compiler. PR #39538 introduced a regression where sorting slices suddenly became slower, since `slice1.lt(slice2)` was much slower than `slice1.cmp(slice2) == Less`. This problem is now fixed. To verify, I benchmarked this simple program: ```rust fn main() { let mut v = (0..2_000_000).map(|x| x * x * x * 18913515181).map(|x| vec![x, x ^ 3137831591]).collect::<Vec<_>>(); v.sort(); } ``` Before this PR, it would take 0.95 sec, and now it takes 0.58 sec. I also tried changing the `is_less` lambda to use `cmp` and `partial_cmp`. Now all three versions (`lt`, `cmp`, `partial_cmp`) are equally performant for sorting slices - all of them take 0.58 sec on the benchmark. Tangentially, as soon as we get `default impl`, it might be a good idea to implement a blanket default impl for `lt`, `gt`, `le`, `ge` in terms of `cmp` whenever possible. Today, those four functions by default are only implemented in terms of `partial_cmp`. r? @alexcrichton
frewsxcv
added a commit
to frewsxcv/rust
that referenced
this pull request
Feb 11, 2017
…d, r=alexcrichton Specialize `PartialOrd<A> for [A] where A: Ord` This way we can call `cmp` instead of `partial_cmp` in the loop, removing some burden of optimizing `Option`s away from the compiler. PR rust-lang#39538 introduced a regression where sorting slices suddenly became slower, since `slice1.lt(slice2)` was much slower than `slice1.cmp(slice2) == Less`. This problem is now fixed. To verify, I benchmarked this simple program: ```rust fn main() { let mut v = (0..2_000_000).map(|x| x * x * x * 18913515181).map(|x| vec![x, x ^ 3137831591]).collect::<Vec<_>>(); v.sort(); } ``` Before this PR, it would take 0.95 sec, and now it takes 0.58 sec. I also tried changing the `is_less` lambda to use `cmp` and `partial_cmp`. Now all three versions (`lt`, `cmp`, `partial_cmp`) are equally performant for sorting slices - all of them take 0.58 sec on the benchmark. Tangentially, as soon as we get `default impl`, it might be a good idea to implement a blanket default impl for `lt`, `gt`, `le`, `ge` in terms of `cmp` whenever possible. Today, those four functions by default are only implemented in terms of `partial_cmp`. r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
ghost
deleted the
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
This way we can call
cmpinstead ofpartial_cmpin the loop, removing some burden of optimizingOptions away from the compiler.PR #39538 introduced a regression where sorting slices suddenly became slower, since
slice1.lt(slice2)was much slower thanslice1.cmp(slice2) == Less. This problem is now fixed.To verify, I benchmarked this simple program:
Before this PR, it would take 0.95 sec, and now it takes 0.58 sec.
I also tried changing the
is_lesslambda to usecmpandpartial_cmp. Now all three versions (lt,cmp,partial_cmp) are equally performant for sorting slices - all of them take 0.58 sec on thebenchmark.
Tangentially, as soon as we get
default impl, it might be a good idea to implement a blanket default impl forlt,gt,le,gein terms ofcmpwhenever possible. Today, those four functions by default are only implemented in terms ofpartial_cmp.r? @alexcrichton