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

Drop glue is always inlined into caller crate #64140

Closed
alexcrichton opened this issue Sep 4, 2019 · 5 comments
Closed

Drop glue is always inlined into caller crate #64140

alexcrichton opened this issue Sep 4, 2019 · 5 comments
Labels
A-codegen Area: Code generation I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

I was investigating the compile time of a crate recently and came across a surprising aspect of the compiler. It looks like the drop glue, or rather the code necessary to drop a type, is inlined unconditionally into the caller crate, even if there are no generics in play.

This program:

pub fn a(_: std::process::Command) {}

generates a whopping 15 thousand lines of LLVM IR. Now Command certainly is a large type and I think it's fine that its drop glue is that big, but this is a concrete type and every single crate which uses Command is generating this 15 thousand lines of IR.

At least in debug mode we shouldn't be doing this for concrete types, but rather the destructor of Command should be compiled into the standard library and then crates which use it should link against this precompiled copy. I would go as far as to argue that this should almost always happen and only when instructed otherwise should the compiler inline destructors across crates (same as for all other methods).

cc @michaelwoerister, do you know if this was a deliberate decision, and is this perhaps something that's relatively easily fixed or something that would be much more intrusive to experiment with?

@alexcrichton alexcrichton added A-codegen Area: Code generation I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 4, 2019
@alexcrichton
Copy link
Member Author

For a smaller reproduction of this:

$ cat bar.rs
pub struct A(String);
$ cat foo.rs
pub fn a(_: bar::A) {}
$ rustc bar.rs --crate-type rlib
$ rustc foo.rs --crate-type rlib --extern bar=libbar.rlib --emit llvm-ir
$ wc -l foo.ll
980 foo.ll

where this is the generated LLVM IR

@michaelwoerister
Copy link
Member

I remember that @arielb1 was tuning inline attributes for drop-glue at some point. It might have been deliberate but I don't remember anything that would make it necessary for correctness. It was probably just easier to implement that way. Seems like something that we should fix though.

bors added a commit that referenced this issue Jan 21, 2020

Verified

This commit was signed with the committer’s verified signature.
gsmet Guillaume Smet
Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0)

This PR adds drop-glue to the set of monomorphizations that can be shared across crates via `-Zshare-generics`.

This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (`upstream_monomorphizations_for(def_id_of_drop_in_place)`). That should be fixable but let's do a perf run first.

Potentially fixes issue #64140. (cc @alexcrichton)
The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal.

r? @ghost
bors added a commit that referenced this issue Jan 22, 2020
Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0)

This PR adds drop-glue to the set of monomorphizations that can be shared across crates via `-Zshare-generics`.

This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (`upstream_monomorphizations_for(def_id_of_drop_in_place)`). That should be fixable but let's do a perf run first.

Potentially fixes issue #64140. (cc @alexcrichton)
The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal.

r? @ghost
bors added a commit that referenced this issue Jan 24, 2020
Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0)

This PR adds drop-glue to the set of monomorphizations that can be shared across crates via `-Zshare-generics`.

This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (`upstream_monomorphizations_for(def_id_of_drop_in_place)`). That should be fixable but let's do a perf run first.

Potentially fixes issue #64140. (cc @alexcrichton)
The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal.

r? @ghost
@michaelwoerister
Copy link
Member

It seems like #68414 might help fixing this but is not sufficient on its own because libstd is compiled with -Zshare-generics=no. I'll do experiments with changing that.

bors added a commit that referenced this issue Jan 30, 2020
…try>

[Experiment] Export generic instances from libstd.

This should resolve issue #64140. However it is unclear if there are detrimental effects. Let's test if there are performance improvements to be had.

r? @ghost
@jonas-schievink
Copy link
Contributor

This seems like a duplicate of #38827

@Mark-Simulacrum
Copy link
Member

Closing in favor of #38827, I agree this is a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants