Faster slice PartialOrd #28436
Faster slice PartialOrd #28436
Conversation
Knowing the result of equality comparison can enable additional optimizations in LLVM. Additionally, this makes it obvious that `partial_cmp` on totally ordered types cannot return `None`.
Reusing the same idea as in rust-lang#26884, we can exploit the fact that the length of slices is known, hence we can use a counted loop instead of iterators, which means that we only need a single counter, instead of having to increment and check one pointer for each iterator. Using the generic implementation of the boolean comparison operators (`lt`, `le`, `gt`, `ge`) provides further speedup for simple types. This happens because the loop scans elements checking for equality and dispatches to element comparison or length comparison depending on the result of the prefix comparison. ``` test u8_cmp ... bench: 14,043 ns/iter (+/- 1,732) test u8_lt ... bench: 16,156 ns/iter (+/- 1,864) test u8_partial_cmp ... bench: 16,250 ns/iter (+/- 2,608) test u16_cmp ... bench: 15,764 ns/iter (+/- 1,420) test u16_lt ... bench: 19,833 ns/iter (+/- 2,826) test u16_partial_cmp ... bench: 19,811 ns/iter (+/- 2,240) test u32_cmp ... bench: 15,792 ns/iter (+/- 3,409) test u32_lt ... bench: 18,577 ns/iter (+/- 2,075) test u32_partial_cmp ... bench: 18,603 ns/iter (+/- 5,666) test u64_cmp ... bench: 16,337 ns/iter (+/- 2,511) test u64_lt ... bench: 18,074 ns/iter (+/- 7,914) test u64_partial_cmp ... bench: 17,909 ns/iter (+/- 1,105) ``` ``` test u8_cmp ... bench: 6,511 ns/iter (+/- 982) test u8_lt ... bench: 6,671 ns/iter (+/- 919) test u8_partial_cmp ... bench: 7,118 ns/iter (+/- 1,623) test u16_cmp ... bench: 6,689 ns/iter (+/- 921) test u16_lt ... bench: 6,712 ns/iter (+/- 947) test u16_partial_cmp ... bench: 6,725 ns/iter (+/- 780) test u32_cmp ... bench: 7,704 ns/iter (+/- 1,294) test u32_lt ... bench: 7,611 ns/iter (+/- 3,062) test u32_partial_cmp ... bench: 7,640 ns/iter (+/- 1,149) test u64_cmp ... bench: 7,517 ns/iter (+/- 2,164) test u64_lt ... bench: 7,579 ns/iter (+/- 1,048) test u64_partial_cmp ... bench: 7,629 ns/iter (+/- 1,195) ```
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
|
Benchmarking data is based on https://gist.github.com/ranma42/51d46e4a4664e388a8f7 |
| let l = cmp::min(self.len(), other.len()); | ||
|
|
||
| for i in 0..l { | ||
| match self[i].cmp(&other[i]) { |
There was a problem hiding this comment.
Are bounds checks eliminated here? This suggests you need to slice self and other to length l for it to be.
There was a problem hiding this comment.
(I guess this wasn't needed in the partialeq case since it requires equal lengths, and llvm understands that)
There was a problem hiding this comment.
You are right, in order to remove all bound checks I need to re-slice self and other, which further improves performance :)
There was a problem hiding this comment.
When you add this, maybe add a comment to indicate why you reslice.
|
@bluss In a previous version my code was using Some(self.cmp(other)), but I was not sure about which form was preferred. I will rewrite as you suggested. |
|
I almost think the main win is not using the iterator comparators' 1) |
|
@bluss Another significant win (which kind of surprised me) was not specialising the boolean comparisons "eagerly". It is faster to only check for equality in the loop and then perform the dispatch "outside" than to have an explicit implementation like #[inline]
fn le(&self, other: &[T]) -> bool {
let l = cmp::min(self.len(), other.len());
let lhs = &self[..l];
let rhs = &other[..l];
for i in 0..l {
match lhs[i].partial_cmp(&rhs[i]) {
Some(Ordering::Equal) => (),
Some(Ordering::Less) => return true, // this
_ => return false, // makes the loop slower
}
}
self.len().le(&other.len())
} |
|
Oh that's cool, thanks for the explanation! |
Instead of manually defining it, `partial_cmp` can simply wrap the result of `cmp` for totally ordered types.
In order to get rid of all range checks, the compiler needs to explicitly see that the slices it iterates over are as long as the loop variable upper bound. This further improves the performance of slice comparison: ``` test u8_cmp ... bench: 4,761 ns/iter (+/- 1,203) test u8_lt ... bench: 4,579 ns/iter (+/- 649) test u8_partial_cmp ... bench: 4,768 ns/iter (+/- 761) test u16_cmp ... bench: 4,607 ns/iter (+/- 580) test u16_lt ... bench: 4,681 ns/iter (+/- 567) test u16_partial_cmp ... bench: 4,607 ns/iter (+/- 967) test u32_cmp ... bench: 4,448 ns/iter (+/- 891) test u32_lt ... bench: 4,546 ns/iter (+/- 992) test u32_partial_cmp ... bench: 4,415 ns/iter (+/- 646) test u64_cmp ... bench: 4,380 ns/iter (+/- 1,184) test u64_lt ... bench: 5,684 ns/iter (+/- 602) test u64_partial_cmp ... bench: 4,663 ns/iter (+/- 1,158) ```
|
Updated with improvements suggested by @bluss (and new benchmarking results). |
|
Yes, I think it's best to leave a comment in the code for the reslicing. It looks redundant for the reader. |
|
It might also be a good idea to avoid declaring |
|
They don't need inline, since the trait impl is already generic. I think it's fine to remove.. we are trying to be conservative with the inlines. |
Be more conservative with inlining.
The explicit slicing is needed in order to enable additional range check optimizations in the compiler.
|
Thank you, it looks good! Great benchmark wins! @bors r+ |
|
📌 Commit 74dc146 has been approved by |
|
⌛ Testing commit 74dc146 with merge 619d874... |
bluss
added
the
relnotes
|
💔 Test failed - auto-mac-32-opt |
|
@bors retry |
This branch improves the performance of Ord and PartialOrd methods for slices compared to the iter-based implementation. Based on the approach used in #26884.
This branch improves the performance of Ord and PartialOrd methods for slices compared to the iter-based implementation.
Based on the approach used in #26884.