Skip to content

/rust

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

trans: Treat generics like regular functions, not like #[inline] function, during CGU partitioning #38944

Merged
merged 5 commits into from Jan 14, 2017

Conversation

Reviewers

@eddyb eddyb

Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@michaelwoerister @rkruppe @nikomatsakis @bors @alexcrichton @eddyb
@michaelwoerister
Copy link
Contributor

michaelwoerister commented Jan 9, 2017

This PR makes generics be treated just like regular functions during CGU partitioning:

  • the function instantiation is placed in a codegen unit based on the function's DefPath,
  • unless it is marked with #[inline] -- which causes a private copy of the function to be placed in every referencing codegen unit.

This has the following effects:

  • Multi codegen unit builds will become faster because code for generic functions is duplicated less.
  • Multi codegen unit builds might have lower runtime performance, since generics are not available for inlining automatically any more.
  • Single codegen unit builds are not affected one way or the other.

This partitioning scheme is particularly good for incremental compilation as it drastically reduces the number of false positives during codegen unit invalidation.

I'd love to have a benchmark suite for estimating the effect on runtime performance for changes like this one.

r? @nikomatsakis

cc @rust-lang/compiler

michaelwoerister added some commits Jan 6, 2017

@michaelwoerister
trans: Disambiguate generic instance symbol names by instantiating cr… 
…ate.

Two crates will often instantiate the same generic functions. Since
we don't make any attempt to re-use these instances cross-crate, we
would run into symbol conflicts for anything with external linkage.

In order to avoid this, this commit makes the compiler incorporate
the ID of the instantiating crate into the symbol hash. This way
equal generic instances will have different symbols names when
used in different crates.
4dca459
@michaelwoerister
metadata: Add is_exported_symbol() method to CrateStore.
02c7b11
@michaelwoerister
trans: Treat generics like regular functions, not like #[inline] func… 
…tions during CGU partitioning.
5f90947
@michaelwoerister
Remove some out-dated comments from CGU partitioning docs.
Loading status checks…
622730c

@rust-highfive rust-highfive assigned nikomatsakis Jan 9, 2017

@rkruppe

This comment has been minimized.

Sign in to view
Copy link
Member

rkruppe commented Jan 9, 2017 •
edited

The first commit (Disambiguate generic instance symbol names by instantiating crate) is very unexpected to me. AFAIK, we currently use comdat linkage to resolve these collisions, which also allows the linker to combine identical instantiation from multiple crates. And except for the mingw linker limitations, I'm not aware of any problems with that. Yet this commit seems to be a complete departure from that strategy. Could you elaborate on the rationale for that change?
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Jan 9, 2017

Could you elaborate on the rationale for that change?

Sure. We've actually used COMDAT sections/WeakODR linkage only for a brief period (between April and June, I think) and then switched to internal copies for all generic instances. Internal linkage has allowed us to use the same symbol names everywhere without getting naming conflicts. The main reason for switching was the MinGW issue but also that WeakODR functions would optimize a lot worse in LLVM. For some reason, LLVM doesn't seem to really trust the "one definition rule".

I think it's a worthwhile goal to get by without COMDAT sections, just to avoid some complexity.
@rkruppe

This comment has been minimized.

Sign in to view
Copy link
Member

rkruppe commented Jan 9, 2017

Interesting! But I still wonder why the current strategy needs to change at all — is this in preparation of trying to reuse instantiations from upstream crates?
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Jan 9, 2017

No, it's primarily to make incremental compilation more effective. Cross-CGU inlining is very bad for re-usability.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jan 10, 2017

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 10, 2017

📌 Commit fc9dfca has been approved by nikomatsakis
@eddyb

eddyb reviewed Jan 10, 2017
View changes

src/librustc_metadata/creader.rs
@@ -302,10 +302,13 @@ impl<'a> CrateLoader<'a> {
crate_root.def_path_table.decode(&metadata)
});

let exported_symbols = crate_root.exported_symbols.decode(&metadata).collect();

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Jan 10, 2017

Member

Huh, I thought I had removed this... At least it's by DefIndex not by symbol name.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 11, 2017

⌛️ Testing commit fc9dfca with merge ffce217...

bors added a commit that referenced this pull request Jan 11, 2017

@bors
Auto merge of #38944 - michaelwoerister:incr-generics-partitioning, r… 
…=nikomatsakis

trans: Treat generics like regular functions, not like #[inline] function, during CGU partitioning

This PR makes generics be treated just like regular functions during CGU partitioning:

+ the function instantiation is placed in a codegen unit based on the function's DefPath,
+ unless it is marked with `#[inline]`  -- which causes a private copy of the function to be placed in every referencing codegen unit.

This has the following effects:
+ Multi codegen unit builds will become faster because code for generic functions is duplicated less.
+ Multi codegen unit builds might have lower runtime performance, since generics are not available for inlining automatically any more.
+ Single codegen unit builds are not affected one way or the other.

This partitioning scheme is particularly good for incremental compilation as it drastically reduces the number of false positives during codegen unit invalidation.

I'd love to have a benchmark suite for estimating the effect on runtime performance for changes like this one.

r? @nikomatsakis

cc @rust-lang/compiler
Loading status checks…
ffce217
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 11, 2017

💔 Test failed - status-travis
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jan 11, 2017

OS/X linker segfault cc #38878
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jan 11, 2017

@bors retry
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 13, 2017

⌛️ Testing commit fc9dfca with merge 29077d1...

bors added a commit that referenced this pull request Jan 13, 2017

@bors
Auto merge of #38944 - michaelwoerister:incr-generics-partitioning, r… 
…=nikomatsakis

trans: Treat generics like regular functions, not like #[inline] function, during CGU partitioning

This PR makes generics be treated just like regular functions during CGU partitioning:

+ the function instantiation is placed in a codegen unit based on the function's DefPath,
+ unless it is marked with `#[inline]`  -- which causes a private copy of the function to be placed in every referencing codegen unit.

This has the following effects:
+ Multi codegen unit builds will become faster because code for generic functions is duplicated less.
+ Multi codegen unit builds might have lower runtime performance, since generics are not available for inlining automatically any more.
+ Single codegen unit builds are not affected one way or the other.

This partitioning scheme is particularly good for incremental compilation as it drastically reduces the number of false positives during codegen unit invalidation.

I'd love to have a benchmark suite for estimating the effect on runtime performance for changes like this one.

r? @nikomatsakis

cc @rust-lang/compiler
Loading status checks…
29077d1
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 13, 2017

💔 Test failed - status-travis
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Jan 13, 2017 •
edited by alexcrichton

@bors retry

OSX linker segfault again :( #38878
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 13, 2017

⌛️ Testing commit fc9dfca with merge ed8f513...

bors added a commit that referenced this pull request Jan 13, 2017

@bors
Auto merge of #38944 - michaelwoerister:incr-generics-partitioning, r… 
…=nikomatsakis

trans: Treat generics like regular functions, not like #[inline] function, during CGU partitioning

This PR makes generics be treated just like regular functions during CGU partitioning:

+ the function instantiation is placed in a codegen unit based on the function's DefPath,
+ unless it is marked with `#[inline]`  -- which causes a private copy of the function to be placed in every referencing codegen unit.

This has the following effects:
+ Multi codegen unit builds will become faster because code for generic functions is duplicated less.
+ Multi codegen unit builds might have lower runtime performance, since generics are not available for inlining automatically any more.
+ Single codegen unit builds are not affected one way or the other.

This partitioning scheme is particularly good for incremental compilation as it drastically reduces the number of false positives during codegen unit invalidation.

I'd love to have a benchmark suite for estimating the effect on runtime performance for changes like this one.

r? @nikomatsakis

cc @rust-lang/compiler
Loading status checks…
ed8f513
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 13, 2017

💔 Test failed - status-travis
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Jan 13, 2017

@bors: retry

  • github was down
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 14, 2017

⌛️ Testing commit fc9dfca with merge ef04fc8...

bors added a commit that referenced this pull request Jan 14, 2017

@bors
Auto merge of #38944 - michaelwoerister:incr-generics-partitioning, r… 
…=nikomatsakis

trans: Treat generics like regular functions, not like #[inline] function, during CGU partitioning

This PR makes generics be treated just like regular functions during CGU partitioning:

+ the function instantiation is placed in a codegen unit based on the function's DefPath,
+ unless it is marked with `#[inline]`  -- which causes a private copy of the function to be placed in every referencing codegen unit.

This has the following effects:
+ Multi codegen unit builds will become faster because code for generic functions is duplicated less.
+ Multi codegen unit builds might have lower runtime performance, since generics are not available for inlining automatically any more.
+ Single codegen unit builds are not affected one way or the other.

This partitioning scheme is particularly good for incremental compilation as it drastically reduces the number of false positives during codegen unit invalidation.

I'd love to have a benchmark suite for estimating the effect on runtime performance for changes like this one.

r? @nikomatsakis

cc @rust-lang/compiler
Loading status checks…
ef04fc8
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 14, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing ef04fc8 to master...

@bors bors merged commit fc9dfca into rust-lang:master Jan 14, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@jonhoo jonhoo referenced this pull request Feb 13, 2017

Open

Performance regression in nightly-2017-01-15 #39773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.