Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy clone semantics #1521

Merged
merged 6 commits into from
May 4, 2016
Merged

Copy clone semantics #1521

merged 6 commits into from
May 4, 2016

Conversation

strega-nil
Copy link

Specifying that <T as Clone>::clone(&t) where T: Copy should be equivalent to ptr::read(&t).

@strega-nil
Copy link
Author

Note that these comments were originally line comments, but that fact seems to have been lost by github.

@briansmith
Copy link

I originally thought this was a good idea, and even suggested something very much like this on the internals mailing list, but I've changed my mind. Every type that implements Copy and Clone should derive Clone and the derived implementation of Clone should be implemented in terms of Copy. Further, the optimizer should be able to notice that a hierarchy of copies is equivalent to a single flattened copy and optimize it to a memcpy. If the optimizer doesn't do that, then it is going to generate terrible code for lots of common cases that aren't calls to Clone::clone such as record updates that modify a single field.

@strega-nil
Copy link
Author

@briansmith The issue isn't derived Clone implementations, it's that we allow you to not derive Clone. If your T: Copy, and you manually implement Clone, this would make it invalid to not make Clone::clone(&t) and ptr::read(&t) equivalent.

The optimizer is also not turned on in debug mode. Allowing us to turn Vec::clone into a memcpy, even in debug mode, would make for much faster code.

Edit: not invalid like undefined behavior or anything, just to be clear.

@strega-nil
Copy link
Author

Note, we've already made breaking changes based on this assumption.

The change in #[derive(Copy, Clone)] to fn clone(&self) -> Self { *self } would break any code that relies on the fact that each of the inner Clone::clones were called.

@briansmith
Copy link

it's that we allow you to not derive Clone

IMO, it is OK to say that if you want Clone to be fast for your Copy type, you need to derive Clone.

The change in #[derive(Copy, Clone)] to fn clone(&self) -> Self { *self } would break any code that relies on the fact that each of the inner Clone::clones were called.

Yes, but IMO that was also a short-term mistake. Once the optimizer is doing the right thing, that shortcut won't be necessary and we can return to the simpler semantics.

In particular, the optimizer can already notice that a loop is equivalent to memcpy. It just needs to get smarter to recognize non-loop equivalents, including recursive invocations of cone/copy, are equivalent to memcpy are equivalent to memcpy too.

Consider let x = y { u32_field: new_value }. How would your proposed optimization improve this situation? It seems it wouldn't, but this kind of pattern is also important to optimize to memcpy + 32-bit memory write.

@strega-nil
Copy link
Author

@briansmith This isn't an optimization. This is specification. It's saying that the standard library (and anyone else) may rely on the fact that a properly implemented Clone::clone for T: Copy == ptr::read. This allows for optimization through specialization, for example, the #[derive(Copy, Clone)] thing, and many other optimizations based around memcpy.

@briansmith
Copy link

Yes, but I'm saying that if the optimizer is working well, then you don't need to specify anything special here. It would be useful to see an example where this rule is needed that can't be done without the rule by simply improving the optimizer.

@Amanieu
Copy link
Member

Amanieu commented Mar 1, 2016

The RFC should specify what the consequences are for types that violate this rule. In particular this must not result in memory unsafety.

@strega-nil
Copy link
Author

@briansmith The issue isn't where the optimizer is working. It's where it isn't. Optimization is not always on, like in debug mode. Also, building a language for a sufficiently smart compiler doesn't seem like the correct way of doing things. Often we will have to specifically specialize.

@strega-nil
Copy link
Author

@Amanieu yes, that is the intention, I will write it down.

@briansmith
Copy link

Why not specify that even in debug mode, the copy coalescing optimizations are done? Then there is no semantic change to the language needed and debug build results are faster.

@strega-nil
Copy link
Author

@briansmith The semantics would still change, and then you would have different semantics between debug and release.

@briansmith
Copy link

To be clear, there's nothing that would change semantically in my definition. The only thing that would change is that debug builds would be specified to have an optimization pass enabled, for at least Clone::clone that they currently don't have enabled, and that optimization pass would be made smarter. The language definition would stay exactly the same.

@strega-nil
Copy link
Author

@briansmith Ah. You can't really do that. The necessary optimizations to realize that:

assert!(src.len() == dst.len());
let len = dst.len();
let src = &src[..len];
for i in 0..len {
    self[i].clone_from(&src[i]);
}

is equivalent to

assert!(src.len() == dst.len());
memcpy(src.as_ptr(), dst.as_mut_ptr(), dst.len());

is... most of them.

# Unresolved questions
[unresolved]: #unresolved-questions

What the exact wording should be.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative wording: "When Copy type has user-defined implementation of Clone it's unspecified whether clone or memcpy is called when cloning a value of this type." or (shamelessly stolen from somewhere) "<...if Copy...> An implementation is allowed to omit the call to clone even if is has side effects. Such elision of calls to clone is called clone elision."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrochenkov it's not that it's unspecified whether clone or ptr::read is called when you actually call .clone(). It's just that libraries (including the standard library) are able to assume that Clone::clone == ptr::read for T: Copy; and, if that requirement is not met, libraries are allowed to exhibit unexpected (but defined) behavior. (Just like if an Ord type doesn't actually have an absolute ordering, or if an Eq type doesn't actually have transitive equality).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not that it's unspecified whether clone or ptr::read is called when you actually call .clone()

Why not? If we go this route anyway, let's give the compiler/libraries all the freedom.
I'd even write "user-defined implementations of Clone for Copy types are ignored", but I'm not sure about generics:

fn f<T: Clone>(arg: T) -> T { arg.clone() } // It's not known if `T` is `Copy` or not and I'm not sure it's convenient to turn `clone`s into `memcpy`s during monomorphisation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that libraries (including the standard library) are able to assume that Clone::clone == ptr::read for T: Copy

derive is not a library, so the language already can omit clones as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, "clone is always ignored" is better than "clone is sometimes ignored" from pedagogical point of view.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm...

At first I thought this was a bad idea, but then I started thinking. Not "always ignored", but if your clone has different semantics from a ptr::read then it should be implementation defined whether it is called.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like the "optimizer magic overriding of clone".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arielb1 Yeah, I guess. I do think it would be better if it's just kept to libraries and language constructs like #[derive]

@glaebhoerl
Copy link
Contributor

Is there any reason to write a manual Clone impl for a type that's Copy in the first place, instead of deriving it? If we want to depend on the two being equivalent at the library level (as proposed), we should lint against it. (If we were to want to depend on it at the language level (as not proposed), then we should just make it an error outright.)

@comex
Copy link

comex commented Mar 1, 2016

@glaebhoerl Unfortunately, derived impls on generic structs sometimes have the wrong trait bounds, so you have to implement them manually.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 1, 2016

@comex: isn't that "just a bug"?

@strega-nil
Copy link
Author

Also, it's possible to implement manually, so it's possible to implement manually wrong. This RFC isn't about all those people who #[derive(Copy, Clone)]. It's about anyone who manually implements Clone, for whatever reason (including the reasons @comex stated, or just reasons of style, or to rely on Copy for older compilers, or even really silly reasons).

@retep998
Copy link
Member

retep998 commented Mar 1, 2016

@ubsan I manually implement Clone in winapi to force the implementation to rely on Copy and thereby improve compile times.

@strega-nil
Copy link
Author

Some people do have to implement manually, like @retep998.

@arielb1
Copy link
Contributor

arielb1 commented Mar 1, 2016

I am not sure how much teeth this RFC has.

We already have both Clone::clone and Clone::clone_from, which may be implemented separately (and allocate differently, even). I don't think that we ever intended to specify which of them is called by standard library functions.

@strega-nil
Copy link
Author

@arielb1 Yes, because it's specified that Clone::clone_from(x, y) shall be equivalent to *x = Clone::clone(y). We have to specify how cloning Copy types should work, or else we must do exactly what we say we are doing (i.e., cloning each individual value).

@arielb1
Copy link
Contributor

arielb1 commented Mar 1, 2016

Right. I think the "equivalent in functionally" language fits this case quite well.

@strega-nil
Copy link
Author

I think that the RFC as written follows what you're saying?

@durka
Copy link
Contributor

durka commented Mar 1, 2016

We did not yet make the #[derive(Copy, Clone)] change cited above, this potential behavior change being one of the reasons. If this language goes in then that obstacle would presumably be removed. However, I have an idea for how to do that optimization better anyway, after specialization stabilizes :)

@strega-nil
Copy link
Author

@bluss ptr::read is specified in the document. It's not defined whether memcpy copies padding bytes.

@Kimundi
Copy link
Member

Kimundi commented Mar 3, 2016

@ubsan My point was not that there should be Clone specific compiler optimizations, but that there could be.

As in, if Clone is specified to not do more than Copy, then it would not matter if the compiler did this since there would be no observable behavior difference.

@arielb1
Copy link
Contributor

arielb1 commented Mar 3, 2016

@Kimundi

I see this as pretty similar to the situation with PartialEq and co. The compiler will not assume that your implementation of PartialEq is symmetric in any way (calls to a == b will not become calls to PartialEq::eq(&b, &a)). However, library functions might get confused.

@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 15, 2016
@e-oz
Copy link

e-oz commented Apr 17, 2016

Hate breaking changes. Have you checked how many cargo crates will be affected? If yes - please share the numbers. From discussion it looks like breaking change just to make debug mode faster, but I think it can't be true.

@retep998
Copy link
Member

@e-oz This won't cause anything to stop compiling. This merely makes an assumption that the standard library and compiler can optimize based on. If you can find any crates in the wild that would be affected negatively by this change, please do bring them up as there isn't really any way to find out in an automated fashion.

@ticki
Copy link
Contributor

ticki commented Apr 17, 2016

I think it is very unlikely that such a change will break anything at all.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 17, 2016

(I also don't consider this a change, merely formally stating an existing assumption.)

Pre-emptive update: I don't really want to get into dickering over what is or is not a breaking change. What I meant here was that there is surely existing code that would exhibit bugs if clone and copy are not aligned (think generics), and that we've used the assumption that copy and clone align in their semantics elsewhere, but failed to document that assumption.

@e-oz
Copy link

e-oz commented Apr 17, 2016

Committed code contains words "this is a breaking change", that's why I asked. Sorry for being suspicious:)

@aturon
Copy link
Member

aturon commented Apr 19, 2016

I'm in favor of documenting these expectations for the semantics of Clone -- clearly, any other behavior for Copy data would be incredibly surprising and likely to lead to bugs.

@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC. However, since the libs team also claims jurisdiction, I'll let them merge it. :)

@alexcrichton
Copy link
Member

The libs team discussed this RFC at triage today and the decision was to merge. It was brought up that the user-facing documentation probably wants to indicate *self instead of ptr::read as it's a bit more understandable, but that doesn't need to change the body of this RFC.

Thanks again for the RFC @ubsan!

Tracking issue: rust-lang/rust#33416

@bluss
Copy link
Member

bluss commented Jan 5, 2017

Vec::clone specialization for T: Copy is realized since rust-lang/rust/pull/38182 (Through extend_from_slice).

@Centril Centril added A-traits-libstd Standard library trait related proposals & ideas A-references Proposals related to references A-convention Proposals relating to documentation conventions. A-machine Proposals relating to Rust's abstract machine. and removed A-machine Proposals relating to Rust's abstract machine. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-convention Proposals relating to documentation conventions. A-references Proposals related to references A-traits-libstd Standard library trait related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.