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

[experimental] Don't partition generic and non-generic code into different CGUs during incr. comp. #53963

Conversation

michaelwoerister
Copy link
Member

At the moment we are splitting generic and non-generic code into different CGUs when partitioning the crate during incremental compilation. The reasoning behind this is that generic code might change more often than non-generic code. That reasoning was never properly validated though. Since it's simple to just not split things, let's give it a try and see how it performs.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2018
@michaelwoerister
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 5, 2018

⌛ Trying commit c481aae427901e3821db90ef91d4f025ac0c497f with merge 01f9f95a596c37b93bbffdc2ef821b761a3e642f...

@michaelwoerister
Copy link
Member Author

cc #53929

@pnkfelix
Copy link
Member

pnkfelix commented Sep 5, 2018

@rust-timer build 01f9f95a596c37b93bbffdc2ef821b761a3e642f

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@michaelwoerister
Copy link
Member Author

@pnkfelix We should wait for the try build to finish anyway.

@michaelwoerister
Copy link
Member Author

@rust-timer build 01f9f95a596c37b93bbffdc2ef821b761a3e642f

@rust-timer
Copy link
Collaborator

Success: Queued 01f9f95a596c37b93bbffdc2ef821b761a3e642f with parent b0297f3, comparison URL.

@bors
Copy link
Contributor

bors commented Sep 5, 2018

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister
Copy link
Member Author

I think we'll have to redo benchmarking after #53962 has been merged. The effects on script-servo and style-servo cases would be rather interesting.

@michaelwoerister
Copy link
Member Author

There are no results yet for script-servo and style-servo but the ones we have don't look too good: https://perf.rust-lang.org/compare.html?start=b0297f3043e4ed592c63c0bcc11df3655ec8cf46&end=01f9f95a596c37b93bbffdc2ef821b761a3e642f&stat=wall-time

@michaelwoerister
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 6, 2018

⌛ Trying commit 4e75f25 with merge 08bf2b2...

bors added a commit that referenced this pull request Sep 6, 2018
[experimental] Don't partition generic and non-generic code into different CGUs during incr. comp.

At the moment we are splitting generic and non-generic code into different CGUs when partitioning the crate during incremental compilation. The reasoning behind this is that generic code might change more often than non-generic code. That reasoning was never properly validated though. Since it's simple to just not split things, let's give it a try and see how it performs.
@nikomatsakis
Copy link
Contributor

@michaelwoerister

The reasoning behind this is that generic code might change more often than non-generic code. That reasoning was never properly validated though. Since it's simple to just not split things, let's give it a try and see how it performs.

It seems like what we are seeing in these benchmarks is basically just "bigger CGU means more time to rebuild", I suspect?

@bors
Copy link
Contributor

bors commented Sep 6, 2018

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister
Copy link
Member Author

Yes, it's a double-edged sword: If the partition is bigger and it is re-used you'll have saved time when generating it (less redundant work done). But you also increase the amount of work that needs to be re-done if it's not re-used because there are more functions in it that need to be re-compiled although it might just be one or two that actually have changed.

@michaelwoerister
Copy link
Member Author

@rust-timer build 08bf2b2

@rust-timer
Copy link
Collaborator

Success: Queued 08bf2b2 with parent 35a5541, comparison URL.

@alexcrichton
Copy link
Member

@michaelwoerister I wonder, could we perhaps ignore #[inline] entirely in debug mode? That may get more concrete instantiations in upstream crates (for example String::deref) to avoid generating code in each CGU? (this wouldn't affect optimized mode of course)

@michaelwoerister
Copy link
Member Author

@alexcrichton I thought that we do that already:

let inline_in_all_cgus =
tcx.sess.opts.debugging_opts.inline_in_all_cgus.unwrap_or_else(|| {
tcx.sess.opts.optimize != OptLevel::No
}) && !tcx.sess.opts.cg.link_dead_code;

@alexcrichton
Copy link
Member

Oh hm perhaps! I guess I was thinking that if you have a crate like:

// crate "A"
#[inline]
pub fn foo() {}

and then depended on that with:

// crate "B"
extern crate a;

pub fn bar() { a::foo(); }

we'll codegen a::foo into B's CGU, right? Instead, though, if we ignored #[inline] entirely then A's CGU would have the code for foo and B would reference that.

(this doesn't apply super well to libstd because it's optimized, so it may not be a good idea in general)

@michaelwoerister
Copy link
Member Author

Yeah, something like that should be doable via the existing -Zshare-generics infrastructure that does the same thing for monomorphizations already.

@michaelwoerister
Copy link
Member Author

I opened #54089, so we don't lose track of the idea regarding inline functions. I'm closing this PR since it didn't really work out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants