perf: override count, nth, nth_back, last and max for BitIterator
#8696
Conversation
| } | ||
|
|
||
| // Go to the one before the last bit | ||
| self.current_offset = self.end_offset - 1; |
There was a problem hiding this comment.
Note to self: we don't need to check for self.end_offset > 0 because if it were zero so would current_offset and the code would exit above 👍
| fn get_value<T: SharedBetweenBitIteratorAndSliceIter>(iter: T) -> Self::Output; | ||
| } | ||
|
|
||
| fn assert_cases<O: Op>() { |
There was a problem hiding this comment.
it would be nice if there were some comments here that explained at a high level what this function did
I think it basically calls various next() / last(), etc functions on two iterators and then asserts they produce the same result
However, it is a very nice setup and easy to follow
| .into_iter() | ||
| } | ||
|
|
||
| fn setup_and_assert( |
There was a problem hiding this comment.
Thank you for the nice tests -- since they are always using BitIterator and Vec<bool> I bet there is a way to reduce some of the generics, but that is just a personal preference.
There was a problem hiding this comment.
In order for the generic to be removed we need to have 2 functions, 1 for BitIterator and 1 for Vec<bool>, I did not want to have 2 functions to avoid possible bugs in the tests where the setup is different.
| let mut expected = expected.clone(); | ||
| for _ in 0..expected.len() { | ||
| let actual_val = actual.nth_back(2); | ||
| let expected_val = expected.nth_back(2); |
There was a problem hiding this comment.
these go off the back of the iterators (which return None) which I think is fine
There was a problem hiding this comment.
Yeah I know, this is on purpose, thanks
|
@rluvaton - please let me know if you would like to address comments in this PR before I merge it |
|
Yes, addressing now, thanks |
|
@alamb updated |
3 participants
Which issue does this PR close?
N/A
Rationale for this change
overriding this function improve performance over the fallback implementation
What changes are included in this PR?
Override implementation of:
countwhich is not optimized away even whenExactSizeIteratoris implementednthto avoid callingnextn + 1times (which is also used when doing.skip)nth_backlastmaxAre these changes tested?
Yes, I've added a lot of tests
Are there any user-facing changes?
Nope