Add Iterator specialisation for Repeat::nth

[varkor]

A final (implementational) follow-up for #47370, this (solely) adds an Iterator::nth optimisation specialisation for Repeat.

This changes the number of times clone() is called on the repeated value, which is an observable change. It's not entirely clear whether this is acceptable, and might benefit from a FCP.

cc @sfackler, @rkruppe, @bluss

[rust-highfive]

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

[varkor]

r? @scottmcm

(Simpler for someone involved in the previous discussion to review.)

[scottmcm]

@varkor I'm not an approved reviewer (and am not on the libs team), so cannot be assigned.

Since this is as much a policy decision as a library change, maybe it would make sense to include as part of this change a documentation comment on Clone describing exactly when eliding a Clone (and its matching Drop) is legal (of which this ends up taking advantage).

Brainstorming for things that might not want this behaviour:

• Scope before/after logging helpers?
• Something using a semaphore to limit the max possible number of live clones from an originating item?

Side note: Copy elision is the only optimization in C++ (until C++14 added a second one, at least) that can change observable behaviour, which I suppose is both a note about how important and how scary it can be 🙂

[scottmcm]

Consider doing this with something that's need_drop, since LLVM already can remove the loop for trivial things like i32: https://play.rust-lang.org/?gist=546bdfadcb744e3aadabf698267b204f&version=nightly

[varkor]

Are you sure the loop is being removed here? https://play.rust-lang.org/?gist=c93f5f86a61ea6af7db1b3c2ab22b66f&version=nightly is terminated for taking too long to execute (which was the intention for this test case as well), which isn't the case after the specialisation.

[scottmcm]

Try https://play.rust-lang.org/?gist=c93f5f86a61ea6af7db1b3c2ab22b66f&version=stable&mode=release, and it doesn't time out. (Now, I'm sure at least one of the builds runs the test in debug, so it'd still catch it, but it's also a Copy type, so copy elision is definitely allowed -- a dead memcpy is certainly removable -- compared to a more complex Clone impl that's the subject of discussion in the issue.)

[varkor]

Ah, got it! I'll adjust the test, thanks!

[varkor]

@scottmcm: Regarding documentation, do you think it makes more sense to mention this on Clone or Repeat (or both)?

On Clone, something like: "There are no guarantees on the number of times functions or traits over Cloneable types are permitted to clone values, unless explicitly specified by the documentation for that function. Consequently, functions may elide calls to clone as desired."
On Repeat: "Repeat may elide calls to clone on its element as an optimisation."

[hanna-kruppe]

On Clone, something like: "There are no guarantees on the number of times functions or traits over Cloneable types are permitted to clone values, unless explicitly specified by the documentation for that function. Consequently, functions may elide calls to clone as desired."

Just so we're clear: Such a broad change to Clone would mark a major semantic change to Rust that certainly requires a whole RFC. The same decision for Clone on types that are also Copy (much less controversial) did go through an RFC.

Arguably an RFC is already required for any form of Clone elision, no matter how restricted, but it's especially clear-cut when changing Clone in general.

[varkor]

Just so we're clear: Such a broad change to Clone would mark a major semantic change to Rust that certainly requires a whole RFC.

Yes, I wanted to avoid going down this route for exactly that reason. I realise such a change would be far too large to be contained in this issue. So far it seems that there's no general rule for Clone with respect to "explicit number of times called", so it seems plausible that documenting the behaviour in specific cases is reasonable — I don't think it's immediately obvious from every trait/method description exactly how many times they're going to call clone, but I'm obviously not so experienced with these issues.

I think it still isn't exactly clear whether even this sort of change too requires a RFC — might be helpful for the libs team to weigh in here.

[llogiq]

I personally think that whoever wants the side effects should use a for loop, while people using iterators opt in to lazy, optimized stream processing.

[scottmcm]

I think I agree with @rkruppe here, on reflection. The docs for the repeat function should mention that items are produced by cloning, but that the implementation may call it fewer times if possible. A general Clone elision option would be cool, but is too big to bite off right now.

@llogiq Unfortunately there are now cases where the iterator way is faster than the for loop way (particularly around chain), so I don't think that's tenable. I'd certainly be fine with "well, maybe if you really care you shouldn't be using Clone" or something, though, since it does feel like there's at least an implied part of its semantics that clones are interchangeable and thus removing calls to it is reasonable.

[hanna-kruppe]

Note that it's not just side effects in Clone that can make Clone elision user-visible, side effects in Drop also matter. For example, I've seen probably dozens of "noisy drop" types (which print something on drop) used to track the order in which temporaries are dropped. This is still unlikely to matter for program correctness, but it's somewhat more likely than Clone side effects to be user-visible and hence confusing.

I don't believe it's terribly likely that Clone elision for Repeat::nth would do material damage to anyone, but it might very well confuse people, and I cannot rule out that it actually affects the behavior of a real, valid (though arguably badly-written) program. The potential benefits are also very small IMO, so I'm opposed to trying to be clever here. (A specialization for T: Copy+Clone would be fine, of course – though probably only beneficial in debug mode)

[llogiq]

I had overlooked drop. Good point. In that case, specializing for I::Item : Clone + Copy seems like a good solution.

[kennytm]

Review ping for you @Kimundi!

Given the comments by @rkruppe, I suppose this PR should be closed, and an RFC should be written specifying whether Clone/Drop elision is allowed/prohibited before we attempt further optimizations in this area?

[hanna-kruppe]

Alternatively, use specialization to only elide clones of types that are Copy. That's uncontroversial and justified by https://github.com/rust-lang/rfcs/blob/master/text/1521-copy-clone-semantics.md

[varkor]

Yeah, probably it's best to do that for now, and then get around to discussing the issue more visibly in an RFC at a later point. I'll fix that soon.

[oberien]

This doesn't add any performance, though, because llvm is already able to optimize all of these cases.

[hanna-kruppe]

Yeah. It probably will improve compile times (slightly) and performance in debug mode (significantly for large n) so it's still worthwhile IMO.

[varkor]

Implementing this just for Copy types produces type inference errors in the compiler (it's really unfortunate that adding implementations this way can do this, but there we go). I'm not quite invested enough to write up an RFC at this point in time, so I'm just going to close the PR. For such a small change, it's generated a lot of discussion, which indicates that more thought is going to be required before it can be made, so there's no point continuing to push it.

[oberien]

Can you point out what you tried and which type inference was broken? I have experienced something similar in #47082 when I tried to specialize Iterator directly (#36262 (comment)). When I instead created a new specialization trait it worked just fine.

[varkor]

It might be possible to achieve that way, but considering how small this change is (when non-Copy types are excluded), it doesn't seem worthwhile to add an extra specialisation trait for the marginal benefit.