[WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867

Manishearth · 2018-01-30T07:55:49Z

(not yet ready for review)

Builds on #47865

fixes #47796

rust-highfive · 2018-01-30T07:56:00Z

r? @nikomatsakis

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

Manishearth · 2018-01-30T11:46:18Z

Fixed a codegen segfault, now it should be working (still need to thoroughly test)

While this should be doing the right thing codegen-wise, there's a soundness hole. Currently this makes us lose the typechecking of "when all type parameters are Clone, all fields must be Clone" which we get for free in the regular custom derive since it generates Clone::clone calls. I'll need to add a wf check that ensures that such tagged enums get checked correctly.

Also, I need to tighten up the rustc_nocopy_clone marker addition so that it only gets added for enums. There are other cleanups which eddyb wishes to make to this code which should land first.

Manishearth · 2018-01-30T13:46:17Z

This seems to work now. Ready for testing on Gecko (cc @bholley).

Todo:

rebase over Cleanup the shim code #47865 which now contains more of the cleanup from here
add the wf check
get it to show the correct spans in the debuginfo
optionally make it so that dump-mir shows in-use shims
tighten the nocopy_clone marker

bors · 2018-01-30T15:11:23Z

☔ The latest upstream changes (presumably #47870) made this pull request unmergeable. Please resolve the merge conflicts.

bholley · 2018-01-30T16:56:21Z

To test this, build mozilla-central with --disable-debug --enable-optimize --enable-release. Then run:

nm --print-size --size-sort --radix=d libxul.so | grep "..Clone"

nikomatsakis · 2018-01-30T17:43:02Z

@Manishearth to be sure, this is ready for review?

Manishearth · 2018-01-30T17:58:36Z

Nope. Though if you want to do a preliminary review go ahead, the main part of the task is done and these patches are unlikely to change aside from the libsyntax stuff (the librustc_mir stuff is done).

Manishearth · 2018-01-30T18:05:48Z

@nikomatsakis #47865 does need review though. And if you do have time, a review of the MIR code here and the overall approach would be very helpful. eddyb's mostly reviewed #47865 already and has gone through most of the code here but he said he wanted you to look at it too.

nikomatsakis · 2018-01-30T18:11:37Z

@Manishearth yep, I saw it's assigned to me, will get to it soon

Manishearth · 2018-01-31T05:27:23Z

So when I did my compile I got a libgkrust.a that was 4 megabytes smaller, but libXUL was 200kb larger .

I tried to do the symbol thing, however with this change there's a slight wrinkle; the shim symbols don't get correctly named. There are a lot of symbols like __ZN4core5clone5Clone5clone17hcccb0e6168f0476aE.39477 that aren't present in the original, and I believe these are the Clone impls. Summing up all the symbols with Clone in them it's a 200kb win, but because the symbols get differently named there's no guarantee that grepping for Clone measures the same thing in both.

Still, this seems promising enough that I should continue pushing on it.

I need to figure out how to get symbols and debuginfo working correctly. I suspect I need to pass the correct Span down, currently we're just passing down the Span of Clone::clone.

Measurement info:

$ git cinnabar git2hg @
5454ed95c82a956009db2f4b04d008ec8753e61e

# for the "before" build
$ rustc -V
rustc 1.25.0-nightly (21882aad7 2018-01-28)

# for the "after" build
$ rustc -V
rustc 1.25.0-dev
$ g rev-parse @
63c3c9398a8e70e4dffbf0f1b3931fcfd7360f0d

Manishearth · 2018-01-31T05:36:50Z

Trying it out on a separate minimal file, the __ZN4core5clone5Clone5clone17hcccb0e6168f0476aE.39477 things are indeed the symbols we're looking for. Now to figure out how to get the correct symbol name attached to them

Manishearth · 2018-01-31T14:16:18Z

@bholley Testing on my Mac, this overall saves 110kb in clone impls, and 61kb in PropertyDeclarationBlock code. A total of 29 functions get optimized by this.

However, libxul did overall get 360kb larger. I was using a nightly to build the "before" version and have a build running to get the "before" version with a minimal diff (i.e. just with and without my changes to rust) and should have results on that soonish.

Manishearth · 2018-01-31T14:52:46Z

On m-c rev 7b46ef2ae1412b15ed45e7d2367ca491344729f7 , on Linux, comparing this PR (ff517a) vs its base,

Patches reduced PropertyDeclaration clone impls by 67kb
Patches reduced total clone impls by 114kb
Patches caused libXUL size increase by 360kb (again, unsure why)

bholley · 2018-01-31T16:46:58Z

@eddyb

Cleanup the shim code - We now write directly to `RETURN_PLACE` instead of creating intermediates - `tuple_like_shim` takes an iterator (used by #47867) - `tuple_like_shim` no longer relies on it being the first thing to create blocks, and uses relative block indexing in a cleaner way (necessary for #47867) - All the shim builders take `dest, src` arguments instead of hardcoding RETURN_PLACE r? @eddyb

bors · 2018-02-05T04:33:05Z

☔ The latest upstream changes (presumably #47865) made this pull request unmergeable. Please resolve the merge conflicts.

shepmaster · 2018-02-09T22:22:34Z

Ping from triage, @Manishearth ! We haven't heard from you in a week or so; will you be able to address the feedback to get this PR out of WIP state soon?

Manishearth · 2018-02-09T23:39:48Z

This PR is currently waiting on #48063

bors · 2018-03-15T02:20:44Z

☔ The latest upstream changes (presumably #47630) made this pull request unmergeable. Please resolve the merge conflicts.

pietroalbini · 2018-03-20T08:50:16Z

@Manishearth #48063 was closed for inactivity, what do you want to do with this?

Manishearth · 2018-03-20T15:11:27Z

close it for now. It is a decent codesize/perf improvement, however for some reason this improvement doesn't carry over to larger codebases so I'm wary of landing this as is.

@eddyb do you want to bring this over the finish line?

rust-highfive assigned nikomatsakis Jan 30, 2018

Manishearth unassigned nikomatsakis Jan 30, 2018

kennytm added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 30, 2018

Manishearth force-pushed the clone-derive-2 branch from 3d31898 to 275dde3 Compare January 30, 2018 09:17

Manishearth mentioned this pull request Jan 30, 2018

[WIP] Make derive(Clone) memcpy on enum variants containing Copy types #47848

Closed

This was referenced Jan 30, 2018

Cleanup the shim code #47865

Merged

derived Clone implementations for many-variant enums are unnecessarily large #47796

Open

Manishearth force-pushed the clone-derive-2 branch from 0369c8a to 63c3c93 Compare January 30, 2018 16:58

Manishearth changed the title ~~[WIP] Make derive(Clone) memcpy on enum variants containing Copy types~~ [WIP] Make derive(Clone) memcpy on enum variants containing only Copy types Feb 5, 2018

Manishearth added 4 commits February 5, 2018 10:27

Stop generating Clone impls when there isn't a derive(Copy)

41e077a

Ask compiler to use shim for rustc_nocopy_clone_marker types

f58f1be

Add enum shim

84558e8

Emit SetDiscriminant before cloning

8594f87

Manishearth added 4 commits February 5, 2018 10:27

Collapse all Copy arms

d35bc21

Correct argument order in tuple_like_shim

05685bd

Use better span when possible

f13d8dc

hack for symbol name

96c3649

Manishearth force-pushed the clone-derive-2 branch from ff5117a to 96c3649 Compare February 5, 2018 15:33

shepmaster added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 9, 2018

shepmaster added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Feb 17, 2018

pietroalbini added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Feb 26, 2018

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 20, 2018

Manishearth closed this Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867

[WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867

Manishearth commented Jan 30, 2018 •

edited

Loading

rust-highfive commented Jan 30, 2018

Manishearth commented Jan 30, 2018

Manishearth commented Jan 30, 2018

bors commented Jan 30, 2018

bholley commented Jan 30, 2018

nikomatsakis commented Jan 30, 2018

Manishearth commented Jan 30, 2018

Manishearth commented Jan 30, 2018

nikomatsakis commented Jan 30, 2018

Manishearth commented Jan 31, 2018

Manishearth commented Jan 31, 2018

Manishearth commented Jan 31, 2018

Manishearth commented Jan 31, 2018 •

edited

Loading

bholley commented Jan 31, 2018

bors commented Feb 5, 2018

shepmaster commented Feb 9, 2018

Manishearth commented Feb 9, 2018

bors commented Mar 15, 2018

pietroalbini commented Mar 20, 2018

Manishearth commented Mar 20, 2018

[WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867

[WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867

Conversation

Manishearth commented Jan 30, 2018 • edited Loading

rust-highfive commented Jan 30, 2018

Manishearth commented Jan 30, 2018

Manishearth commented Jan 30, 2018

bors commented Jan 30, 2018

bholley commented Jan 30, 2018

nikomatsakis commented Jan 30, 2018

Manishearth commented Jan 30, 2018

Manishearth commented Jan 30, 2018

nikomatsakis commented Jan 30, 2018

Manishearth commented Jan 31, 2018

Manishearth commented Jan 31, 2018

Manishearth commented Jan 31, 2018

Manishearth commented Jan 31, 2018 • edited Loading

bholley commented Jan 31, 2018

bors commented Feb 5, 2018

shepmaster commented Feb 9, 2018

Manishearth commented Feb 9, 2018

bors commented Mar 15, 2018

pietroalbini commented Mar 20, 2018

Manishearth commented Mar 20, 2018

Manishearth commented Jan 30, 2018 •

edited

Loading

Manishearth commented Jan 31, 2018 •

edited

Loading