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

Implement specialised clone_from via #[derive(Clone)] #27939

Closed
wants to merge 9 commits into from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Aug 22, 2015

This also adds machinery to derive so that it can track the mutability of self parameters, which is required so that it can generate the correct match expressions and destructuring.

Fixes #13281

@rust-highfive
Copy link
Collaborator

r? @brson

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

@alexcrichton
Copy link
Member

Looks like make tidy failed, and can you also be sure to add a test for this?

I'm also a little worried about this in terms of compile times, #[derive(Clone)] is probably one of the most common derive modes and having it generate more code runs a real risk of increasing compilation times without much benefit (e.g. this is a very rarely called method). It may be worth benchmarking the difference in compile time for crates with lots of #[derive(Clone)] to get a handle on what the impact is going to be.

@tbu-
Copy link
Contributor

tbu- commented Aug 22, 2015

We've kept this method, maybe we should support it too...

@Diggsey
Copy link
Contributor Author

Diggsey commented Aug 23, 2015

@alexcrichton
I've fixed the formatting, extended the existing #[derive(Clone)] tests to also check clone_from, and added a regression test which specifically checks that clone_from is implemented independently of clone.

I'm not sure how best to benchmark the impact this has on compile times (or whether such results would even be meaningful if I tried to benchmark on my laptop, I seem to get incredibly variable compile times in msys).

I agree with @tbu- if this method exists it should be implemented, otherwise it's just giving the illusion that there's a more performant version of clone.

I've also found clone_from to be essential in some circumstances in order to get the best performance while keeping the borrow checker happy (when writing simulations/games it allows one to keep an immutable copy of the world state from the previous step around at all times without requiring massive amounts of allocation to keep it up to date).

@alexcrichton
Copy link
Member

Can you generate a test cases along the lines of:

  • 1000 structs with #[derive(Clone)]
  • A struct with 1000 fields and #[derive(Clone)]

I'm not saying we should not do this, I'm saying that if this doubles compile times then we should think through this before hastily merging.

@brson
Copy link
Contributor

brson commented Aug 24, 2015

I see @alexcrichton mentioning downsides here, but the advantage of doing this isn't clear. The linked issue doesn't describe any concrete wins. Are there theoretical performance improvements? Can they be measured?

@tbu-
Copy link
Contributor

tbu- commented Aug 24, 2015

@brson Consider a struct containing a vector, with this derive the Clone::clone_from method of that struct won't just throw away its old vector and allocate a new one, but would rather reuse the old storage.

@Kimundi
Copy link
Member

Kimundi commented Aug 24, 2015

I agree with @tbu- here - clone_from is mostly useless if deriving doesn't actually generate code for it.

@brson
Copy link
Contributor

brson commented Aug 28, 2015

Waiting to see how this affects compiletimes per @alexcrichton.

@Diggsey
Copy link
Contributor Author

Diggsey commented Aug 31, 2015

I ran the two tests @alexcrichton described, code: https://gist.github.com/Diggsey/7268beefb47a2cc9a740

Each test was run five times, and the best result used (all times are in seconds):

              before       after
test_0.rs       2.08        7.43        (struct with 1000 fields)
test_1.rs       1.90        3.23        (1000 structs)

So clearly this does have a large negative impact on compile times.

However, in real code it's extremely rare to only derive Clone: normally you'd at least be deriving Debug or PartialEq as well, so I reran the same tests, but with #derive(Clone, Debug).

              before       after
test_0.rs          -           -        (struct with 1000 fields)
test_1.rs       4.02        5.36        (1000 structs)

Unfortunately, rustc overflowed its stack for test_0.rs, both before and after this change.

@alexcrichton
Copy link
Member

Hm ok, so it looks like this is about a 2x-ish hit (which kinda makes sense) in terms of adding compile time to derive(Clone). It's good to know that it's not outlandishly more than that, but could you also try this out on an idiomatic crate with lots of derives? The example that comes to mind for me is winapi, but there may be others as well.

@Diggsey
Copy link
Contributor Author

Diggsey commented Aug 31, 2015

Testing on winapi, compile-time increases from ~27s to ~34s with the addition of clone_from.

@alexcrichton
Copy link
Member

I'm personally feeling that these numbers are significant enough and the benefit is marginal enough that I would prefer to not go forward with this just yet. Perhaps in the future we can enable faster codegen of derive modes for example, but it doesn't seem like the wins outweigh the losses for now to me.

@Aatch
Copy link
Contributor

Aatch commented Sep 25, 2015

One potential option could be a way of opting in to a derived clone_from. Not sure how it would look, #[derive(Clone(with_clone_from))] maybe? Doesn't really matter. This way it doesn't kill compile times but does mean that types can benefit from it. I'm thinking that wrapper types for collections are the best example.

@brson
Copy link
Contributor

brson commented Nov 23, 2015

Since this has been stalled a long time, closing per @alexcrichton's reasoning.

@orlp
Copy link
Contributor

orlp commented Jul 3, 2021

It has been 5 years. Could this be revisited?

@frankmcsherry
Copy link
Contributor

Per the above mention, we needed to manually implement Clone for various wrapper types. Worse, imo, we needed to discover that we had to manually implement Clone for wrapper types. At least, it might help to document that derive(Clone) does not currently derive an implementation of clone_from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[derive(Clone)] should explicitly override clone_from
9 participants