Streamline iterator chaining when computing successors. #148054
Conversation
There are numerous unnecessary `into_iter` calls. Also add a comment explaining why the code looks like this, because it's non-obvious at first glance.
rustbot
added
S-waiting-on-review
|
Huh. Neat. I agree that the iterator chaining thing is subtle, I was very confused by the type the first time I worked on this module. @bors r+ rollup |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - #148016 (Revert constification of `Borrow` and `Deref for Cow` due to inference failure) - #148021 ([rustdoc] Simplify module rendering and HTML tags handling) - #148039 (Add myself to the review rotation) - #148042 (test(frontmatter): Cover spaces between infostring parts) - #148054 (Streamline iterator chaining when computing successors.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #148054 - nnethercote:chain, r=saethlin Streamline iterator chaining when computing successors. There are numerous unnecessary `into_iter` calls. Also add a comment explaining why the code looks like this, because it's non-obvious at first glance. r? `@saethlin`
|
Bors hasn't noticed that this was merged. @bors r- |
bors
added
S-waiting-on-author
|
This appears to have somehow caused a bunch of perf regressions: #148059 (comment) Assuming the regression is real and not some weird noise, the only possible explanation I can think of is that this PR does slightly change the shape of the iterator, and maybe that's enough to cause measurably worse codegen despite giving equivalent results? |
|
Huh, weird about the perf. I'll take a look on Monday, and I will revert it if there's no simple solution. |
I was considering a revert myself, but after seeing this message I'll just keep my hands off and leave things up to you. 👍 Note that a version bump (#148077) will probably land before any fix/revert, so the fix/revert might want a beta backport. |
|
Here’s a perf run confirming that a revert would resolve the regression: #148086 (comment) |
| pub fn successors_for_value(&self, value: u128) -> Successors<'_> { | ||
| let target = self.target_for_value(value); | ||
| (&[]).into_iter().copied().chain(Some(target).into_iter().chain(None)) | ||
| (&[]).into_iter().copied().chain(Some(target)).chain(None) |
There was a problem hiding this comment.
Note that the iterator type changed here, something like Chain<Copied<slice::Iter>, Chain<OptIter, OptIter>> to Chain<Chain<Copied<slice::Iter>, OptIter>, OptIter>.
This slice is empty anyway, but for the rest of the methods returning the same type, it probably hurts access to the slice items to go through two Chain layers.
| pub fn successors_for_value(&self, value: u128) -> Successors<'_> { | ||
| let target = self.target_for_value(value); | ||
| (&[]).into_iter().copied().chain(Some(target).into_iter().chain(None)) | ||
| (&[]).into_iter().copied().chain(Some(target)).chain(None) |
There was a problem hiding this comment.
Some of the original explicit into_iter() could still be avoided with std::iter::chain, plus iter() for slices:
| (&[]).into_iter().copied().chain(Some(target)).chain(None) | |
| [].iter().copied().chain(chain(Some(target), None)) |
6 participants
There are numerous unnecessary
into_itercalls.Also add a comment explaining why the code looks like this, because it's non-obvious at first glance.
r? @saethlin