Elide clone operation when calling iter::Cloned::advance_{back}_by()

[the8472]

This is a change in user-visible behavior. It will not only affect advance_by (which is still unstable) but also the stable nth, nth_back and Skip, Take and other adapters that make use of these methods.
It is also a bit of a test-balloon, if it gets approved there are a few more places where other side-effects could be skipped too.

I'm not sure if it is needed at this stage, but a note about side-effects could also be added to the std::iter module.

Part of #77404

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[cuviper]

It is also a bit of a test-balloon, if it gets approved there are a few more places where other side-effects could be skipped too.

Skipping clone is possibly okay, but I would be surprised to see map closures skipped, for instance.

[the8472]

I would be surprised to see map closures skipped, for instance.

Ideally it would be limited pure(ish) closures, but the trait system can't express that so it would have to be some approximation based on Fn and !&mut and I haven't even prototyped it yet so I think we can leave the discussion to the future. I just wanted to note the intent.

[the8472]

After the discussion in #92543 I took another look at iter::Cloned and if we're already eliding side-effects then last() and count() could also be overridden either bei skipping cloning in all cases or conditionally on T: Copy, via specialization.

I could do that here or in a separate PR.

[m-ou-se]

Seems fine to me, but let's quickly get some more eyes from @rust-lang/libs-api on this. :)

[llogiq]

rust-lang/rust-clippy#8203 attempts to tackle the perf footgun from the other side. I'm not on libs, but I would favor a two-pronged approach where we try to elide clones where possible and have the lint help people deal with the remaining cases.

With that said, I see that the current behavior is in place since 2015. However unlikely, somewhere someone could have implemented a side-effectful Clone impl and rely on all iterated elements being cloned. I would suggest at least a crater run before this is merged, so that if that someone has published on crates.io we can at least contact them before we break their code.

[the8472]

I would suggest at least a crater run before this is merged, so that if that someone has published on crates.io we can at least contact them before we break their code.

Betas already get crater runs. Not sure if it's worth doing a separate one for this.

[joshtriplett]

I'm personally hesitant to merge this, even with a crater run.

It seems to violate compositional reasoning: if people know what .cloned() does, and know what .last() does, they would then know what .cloned().last() does. (And likewise for nth and similar.)

I absolutely think it's reasonable to optimize these away for .copied(), because we know Copy can't have a side effect. But Clone absolutely can.

This is less a question of "will we break some existing code", and more that it seems like especially surprising behavior.

Are there real-world workloads in which people call .cloned() and then do operations on it that could skip the clones? What led to wanting this optimization?

(Also, whether we make this change or not, I don't think we should add the comments suggesting we'd skip other things that are more likely to have side effects, such as closures.)

[the8472]

It seems to violate compositional reasoning: if people know what .cloned() does, and know what .last() does, they would then know what .cloned().last() does.

That depends on what the simplified mental model of cloned() is. If you think of it in terms of only wrapping next() and then running the default implementations of Iterator then yeah.
But if the idea is "it returns cloned items" then when there's no item returned no cloning needs to happen.

The problem with "know what X does" is kind of oversimplifying, because what it really does is return an adapter and adapters are more complicated than just next().

Are there real-world workloads in which people call .cloned() and then do operations on it that could skip the clones?

They're common mistakes. I mostly but not exclusively see them in advent-of-code style exercises or test code, but that suggests that a naive model that people have is only about the outcome of the iterator, not about the order in which adapters are applied. And it often is a very convenient model, a "sufficiently smart compiler std" kind of thing.

https://github.com/Simula-UiB/CRHS/blob/06ae94effdbad20e5ae0fa1fd97b8b10a7a7381d/cryptapath/src/targets/miniaes4x4.rs#L58-L63

And it can be more subtle than that. E.g. cloned().zip(...).next_back() does extra work in trimming iterators of unequal length until they're equal length before returning the tail item, and that trimming would lead to extra clones.

(Also, whether we make this change or not, I don't think we should add the comments suggesting we'd skip other things that are more likely to have side effects, such as closures.)

Well, that's what I'd try to do next, optimize away map and inspect side-effects when combined with zip, skip, take etc. Only for Fn closures and non-&mut items of course 😇 (assuming I can get specialization to do this).
And it's not only about the standard library, it would also allow 3rd-party adapters to implement advance_by in a way that skips unnecessary work.

[llogiq]

@joshtriplett I hear you. While there is no guarantee of side effects in iterator operations in general, code might reasonably depend on them as the current situation has effectively been stable since 2015.

On the other hand, while we will soon have a clippy lint that can reduce the impact of this problem, it can only reason locally and thus will always miss some cases. And avoiding alloc churn is an attractive optimization.

So while I can certainly relate to your hesitation, we may want to at least think of a path to include these optimizations, perhaps as part of the next future edition. While not as good as getting the performance right now, it would at least give us a future to await (pun totally intended) and in the meantime we can test & benchmark with this PR.

[the8472]

Closing this per discussion in this week's libs meeting. To summarize:

• the change by itself might be ok, but on its own it's likely not worth it
• a stepping stone towards more aggressive iterator optimizations (such as eliding non-essential map side-effects, even conditionally) it's too controversial as long as those optimizations would violate the as-if rule
• if the typesystem allowed us to reliably identify side-effect-free functions (const? pure?) or we could better hint to llvm that something isn't a relevant side-effect that might be an alternative

[llogiq]

Should we then document the current behavior or at least make clear that we don't offer any performance optimization at least for this edition? I might reopen #92543.

[the8472]

Well, if we might want to make another attempt in the future then it might cause less confusion by not saying too much. On the other hand the guidance to avoid unnecessary clones can be useful. So maybe it would be better to phrase it in a way focusing on the latter part.