Specialize Iterator::eq{_by} for TrustedLen iterators
#137122
Conversation
rustbot
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This comment was marked as resolved.
This comment was marked as resolved.
|
Ok, sorry for the noise, but I got confused and then re-confused and then finally (hopefully) un-confused. This optimization is ok on |
yotamofek
changed the title
Iterator::{eq|cmp|partial_cmp}_by for TrustedLen iteratorsIterator::eq{_by} for TrustedLen iterators
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
… r=<try>
Specialize `Iterator::eq{_by}` for `TrustedLen` iterators
I'm sure I got some stuff wrong here, but opening this to get feedback and make sure it's a viable idea at all.
### Motivation
I had a piece of code that open-coded `Iterator::eq`, something like:
```rust
if current.len() != other.len()
|| current.iter().zip(other.iter()).any(|(a, b)| a != b) { ... }
```
... where both `current` and `other` are slices of the same type.
Changing the code to use `current.iter().eq(other)` made it a lot slower, since it wasn't checking the length of the two slices beforehand anymore, which in this instance made a big difference in perf. So I thought I'd see if I can improve `Iterator::eq`.
### Questions
1. I can't specialize for `ExactSizeIterator`, I think it's a limitation of `min_specialization` but not sure exactly why. Is specializing for `TrustedLen` good enough?
2. Should I make a codegen test for this? If so, then how? (I manually checked the assembly to make sure it works as expected)
3. Where should I put `SpecIterCompare`?
4. Can I get a perf run for this, please? I think the compiler uses this in a few places, so it might have an affect.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
| where | ||
| F: FnMut(Self::Item, <B as Iterator>::Item) -> ControlFlow<T>, | ||
| { | ||
| if let (_, Some(a)) = self.size_hint() |
There was a problem hiding this comment.
pondering: What if, instead of specialization, iter_compare just always checked if the size_hints? That way it can work even for things that are neither exact nor trusted.
Something with a size hint of (2, Some(10)) can't possibly be equal to one with (14, None), for example.
And for ESIs, which almost always return let len = self.len(); (len, Some(len)), the compiler can probably optimize the two checks into one. And for the default (0, None) hint the compiler will optimize away the check because obviously the other one can't be shorter than zero.
There was a problem hiding this comment.
Thing is - I'm not sure it would be acceptable if Iterator::eq started returning wrong results due to "wrong" size hints. It won't be UB, but I don't think any other iterator combinators can return wrong results because of a buggy size_hint(). IMHO even ESI's guarantee (which is less than TrustedLen's) might not be strong enough.
There was a problem hiding this comment.
I think it would be equivalent to, say, for_each not being run for the last elements of an iterator if its upper bound hint is incorrectly smaller than the number of elements it can yield.
There was a problem hiding this comment.
An incorrect size_hint is absolutely enough to trigger garbage-in-garbage-out. The default size_hint of (0, None) is always correct, so this would only be an issue if someone explicitly overrides it incorrectly, and that's not allowed.
Of course it's not allowed to be UB if someone implements size_hint wrong, but implementing size_hint wrong is no different from implementing fold wrong, for example: it's 100% allowed to make other things misbehave.
There was a problem hiding this comment.
That's a really good point. Hadn't looked at it that way.
But I do think that as a user, I would be much more surprised if an incorrect size_hint implementation caused anything other than wrong estimations for with_capacity or something, rather than that an incorrect fold will just blow everything up. Maybe the word "hint" makes it sound less consequential. 🤷
Anyways, @the8472 's concern about eliding side effects might be a deal breaker, so I'll wait for the libs team decision on that before trying out different approaches for implementations.
There was a problem hiding this comment.
Yeah, part of the problem is that, today, the size_hint is really only used by collect, which doesn't even use the upper part of it, just the bottom part.
I often wish there was instead just a suggested_reserve() -> usize or something that was more obviously both 1) just for the collect case, and 2) explicitly documented as allowed to be garbage.
|
Finished benchmarking commit (61b77a2): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 7.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.1%, secondary 0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 774.698s -> 773.335s (-0.18%) |
rustbot
removed
the
S-waiting-on-perf
|
r? @the8472 |
|
Personally I think this would be great from a performance perspective but it has the consequence of eliding side-effects of the iterators. Last time I proposed a change that would elide some sideeffects of iterators it got NACK'd by the team. Nominating for discussion since perhaps this case is different since the eq impl involves some non-trivial variability in the amount of items that would be consumed before iteration stops. |
the8472
added
the
I-libs-nominated
Thanks, that's a good point, hadn't occurred to me. I guess one way to get around that is to add a |
|
We discussed this during today's libs meeting, albeit with limited attendance. @rfcbot fcp merge |
|
Team member @the8472 has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns: Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
Let's formally register that... @rfcbot concern side-effects Can we get a crater-test run to gauge this? |
Amanieu
removed
the
I-libs-nominated
|
@bors try |
… r=<try>
Specialize `Iterator::eq{_by}` for `TrustedLen` iterators
I'm sure I got some stuff wrong here, but opening this to get feedback and make sure it's a viable idea at all.
### Motivation
I had a piece of code that open-coded `Iterator::eq`, something like:
```rust
if current.len() != other.len()
|| current.iter().zip(other.iter()).any(|(a, b)| a != b) { ... }
```
... where both `current` and `other` are slices of the same type.
Changing the code to use `current.iter().eq(other)` made it a lot slower, since it wasn't checking the length of the two slices beforehand anymore, which in this instance made a big difference in perf. So I thought I'd see if I can improve `Iterator::eq`.
### Questions
1. I can't specialize for `ExactSizeIterator`, I think it's a limitation of `min_specialization` but not sure exactly why. Is specializing for `TrustedLen` good enough?
2. Should I make a codegen test for this? If so, then how? (I manually checked the assembly to make sure it works as expected)
3. Where should I put `SpecIterCompare`?
4. Can I get a perf run for this, please? I think the compiler uses this in a few places, so it might have an affect.
|
☀️ Try build successful - checks-actions |
|
@craterbot run mode=build-and-test |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
I'm not sure I know how to parse crater's results, but it does seem like a lot of the errors are due to lack of disk space, a few look like flakey tests, and I couldn't find any errors that look related to this PR (haven't found a crate that actually calls Does anyone have any pointers? |
bump still looking for help with deciphering the crater results here |
Yeah that is normal and expected. If you went through all the failed 'root results' and didn't find any failure relating to this change: congrats, you have a clean crater run. :) |
Found in this crater run: rust-lang/rust#137122 Certain randomness causes tests to fail
|
I've been slowly going through all the failures crater found, and so far have not found a single true-positive one that broke because of this PR. |
|
@bors2 try |
Specialize `Iterator::eq{_by}` for `TrustedLen` iterators
<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.
This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using
r? <reviewer name>
-->
<!-- homu-ignore:end -->
I'm sure I got some stuff wrong here, but opening this to get feedback and make sure it's a viable idea at all.
### Motivation
I had a piece of code that open-coded `Iterator::eq`, something like:
```rust
if current.len() != other.len()
|| current.iter().zip(other.iter()).any(|(a, b)| a != b) { ... }
```
... where both `current` and `other` are slices of the same type.
Changing the code to use `current.iter().eq(other)` made it a lot slower, since it wasn't checking the length of the two slices beforehand anymore, which in this instance made a big difference in perf. So I thought I'd see if I can improve `Iterator::eq`.
### Questions
1. I can't specialize for `ExactSizeIterator`, I think it's a limitation of `min_specialization` but not sure exactly why. Is specializing for `TrustedLen` good enough?
2. Should I make a codegen test for this? If so, then how? (I manually checked the assembly to make sure it works as expected)
3. Where should I put `SpecIterCompare`?
4. Can I get a perf run for this, please? I think the compiler uses this in a few places, so it might have an affect.
|
crater gained a new category "prepare-fail" which should now filter out some of those failures. |
|
@craterbot run mode=build-and-test |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
I'm sure I got some stuff wrong here, but opening this to get feedback and make sure it's a viable idea at all.
Motivation
I had a piece of code that open-coded
Iterator::eq, something like:... where both
currentandotherare slices of the same type.Changing the code to use
current.iter().eq(other)made it a lot slower, since it wasn't checking the length of the two slices beforehand anymore, which in this instance made a big difference in perf. So I thought I'd see if I can improveIterator::eq.Questions
ExactSizeIterator, I think it's a limitation ofmin_specializationbut not sure exactly why. Is specializing forTrustedLengood enough?SpecIterCompare?