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

rustc: Fix regression where jemalloc isn't used #57287

Merged
merged 1 commit into from Jan 6, 2019

Conversation

Projects
None yet
7 participants
@alexcrichton
Copy link
Member

alexcrichton commented Jan 2, 2019

In #56986 the linkage of jemalloc to the compiler was switched from the
driver library to the rustc binary to ensure that only rustc itself uses
jemalloc. In doing so, however, it turns out jemalloc wasn't actually
linked in at all! None of the symbols were referenced so the static
library wasn't used. This means that jemalloc wasn't pulled in at all.

This commit performs a bit of a dance to reference jemalloc symbols,
attempting to pull it in despite LLVM's optimizations.

Closes #57115

@alexcrichton alexcrichton force-pushed the alexcrichton:fix-jemalloc branch from 9c95ff4 to 5265c31 Jan 2, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 2, 2019

r? @nikomatsakis

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 2, 2019

@bors: try

bors added a commit that referenced this pull request Jan 2, 2019

Auto merge of #57287 - alexcrichton:fix-jemalloc, r=<try>
rustc: Fix regression where jemalloc isn't used

In #56986 the linkage of jemalloc to the compiler was switched from the
driver library to the rustc binary to ensure that only rustc itself uses
jemalloc. In doing so, however, it turns out jemalloc wasn't actually
linked in at all! None of the symbols were referenced so the static
library wasn't used. This means that jemalloc wasn't pulled in at all.

This commit performs a bit of a dance to reference jemalloc symbols,
attempting to pull it in despite LLVM's optimizations.

Closes #57115
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 2, 2019

⌛️ Trying commit 5265c31 with merge 3550c27...

Show resolved Hide resolved src/rustc/rustc.rs Outdated
Show resolved Hide resolved src/rustc/rustc.rs Outdated
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 3, 2019

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 3, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 3, 2019

Success: Queued 3550c27 with parent ec19464, comparison URL.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 3, 2019

So my take on this:

  • The code itself seems "obviously" "fine" (as in, not going go to break things)
  • But oh god what a hack

It seems surprising to me that there is no more elegant way to do this.

I'm willing therefore to r+ though I might prefer if somebody else who was closer to the whole linking story did so, since maybe they will have another idea. (e.g., @petrochenkov is asking good questions here).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 3, 2019

Also, the try build is ready, not sure what use you had in mind, @alexcrichton ?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 3, 2019

The most elegant way to do this without looking like a hack is probably to use linker flags directly, but actually doing that in Rust would look like more of a hack unfortunately. We don't have that level of control over how things are built to know what's where on the command line, so I've found symbol references to be much more flexible.

Also sorry should have mentioned, but I'd like to do a perf run to confirm that this actually fixes the regression this claims to fix.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 3, 2019

(I'd also like to add more comments in the code about the hacks here code-wise, was just waiting on perf)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 3, 2019

Finished benchmarking try commit 3550c27

@alexcrichton alexcrichton force-pushed the alexcrichton:fix-jemalloc branch from 5265c31 to cd05e6a Jan 3, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 3, 2019

Yay regression is indeed fixed!

I've added some comments, and perhaps in addition to @petrochenkov @michaelwoerister may want to take a look?

rustc: Fix regression where jemalloc isn't used
In #56986 the linkage of jemalloc to the compiler was switched from the
driver library to the rustc binary to ensure that only rustc itself uses
jemalloc. In doing so, however, it turns out jemalloc wasn't actually
linked in at all! None of the symbols were referenced so the static
library wasn't used. This means that jemalloc wasn't pulled in at all.

This commit performs a bit of a dance to reference jemalloc symbols,
attempting to pull it in despite LLVM's optimizations.

Closes #57115

@alexcrichton alexcrichton force-pushed the alexcrichton:fix-jemalloc branch from cd05e6a to ccbf28e Jan 3, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 3, 2019

Other crates that want to use static jemalloc will have to do something like this as well?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 3, 2019

In theory, yes, but in practice it isn't much of a problem. Almost all Rust code that isn't rustc is built with all Rust code statically linked, so the references to the jemalloc symbols will come through things like libstd or just bland Vec::push

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 4, 2019

@alexcrichton

I think the --whole-archive with -l vs #[link] is probably a bug actually?

Some update: apparently I misremembered the details, command line and attribute behave identically (that's good).

The difference was that static uses whole-archive, but static-nobundle doesn't.
That's also not entirely correct since bundling and whole-archive-ness are somewhat independent.
For example, for linking jemalloc in this case we'd want exactly nobundle + whole-archive.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 4, 2019

From my experience at least creating a composable system where libraires can specify linkage and whatnot is insanely hard to say the least and likely downright impossible. We do our best when we can and otherwise just try to fix things when a bug comes up. We're definitely not consistent though :(

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 4, 2019

@bors r+

Since it seems like @petrochenkov doesn't have any blocking objections, I'm going to r+ here for now. Feel free to r- or r? someone else though.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 4, 2019

📌 Commit ccbf28e has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 6, 2019

⌛️ Testing commit ccbf28e with merge af2c159...

bors added a commit that referenced this pull request Jan 6, 2019

Auto merge of #57287 - alexcrichton:fix-jemalloc, r=nikomatsakis
rustc: Fix regression where jemalloc isn't used

In #56986 the linkage of jemalloc to the compiler was switched from the
driver library to the rustc binary to ensure that only rustc itself uses
jemalloc. In doing so, however, it turns out jemalloc wasn't actually
linked in at all! None of the symbols were referenced so the static
library wasn't used. This means that jemalloc wasn't pulled in at all.

This commit performs a bit of a dance to reference jemalloc symbols,
attempting to pull it in despite LLVM's optimizations.

Closes #57115
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 6, 2019

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

@bors bors merged commit ccbf28e into rust-lang:master Jan 6, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment