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

mk: Move disable-jemalloc logic into makefiles #31846

Merged
merged 1 commit into from
Feb 26, 2016

Conversation

alexcrichton
Copy link
Member

The --disable-jemalloc configure option has a failure mode where it will
create a distribution that is not compatible with other compilers. For example
the nightly for Linux will assume that it will link to jemalloc by default as
an allocator for executable crates. If, however, a standard library is used
which was built via ./configure --disable-jemalloc then this will fail
because the jemalloc crate wasn't built.

While this seems somewhat reasonable as a niche situation, the same mechanism is
used for disabling jemalloc for platforms that just don't support it. For
example if the rumprun target is compiled then the sibiling Linux target also
doesn't have jemalloc. This is currently a problem for our cross-build nightlies
which build many targets. If rumprun is also built, it will disable jemalloc for
all targets, which isn't desired.

This commit moves the platform-specific disabling of jemalloc as hardcoded logic
into the makefiles that is scoped per-platform. This way when configuring
multiple targets without the --disable-jemalloc option specified all
targets will get jemalloc as they should.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@alexcrichton
Copy link
Member Author

r? @brson

Unfortunately this all seems basically super brittle because... makefiles. I've tested though by having a local change that disables jemalloc for i686-unknown-linux-gnu, and I then configured both that + x86_64 and ran a full make check until everything passed. That at least gives me a relatively high amount of confidence in this!

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis Feb 23, 2016
@brson
Copy link
Contributor

brson commented Feb 25, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2016

📌 Commit 0af6b65 has been approved by brson

bors added a commit that referenced this pull request Feb 25, 2016
@bors
Copy link
Contributor

bors commented Feb 26, 2016

⌛ Testing commit 0af6b65 with merge 996ea14...

@bors
Copy link
Contributor

bors commented Feb 26, 2016

💔 Test failed - auto-win-msvc-64-opt

The `--disable-jemalloc` configure option has a failure mode where it will
create a distribution that is not compatible with other compilers. For example
the nightly for Linux will assume that it will link to jemalloc by default as
an allocator for executable crates. If, however, a standard library is used
which was built via `./configure --disable-jemalloc` then this will fail
because the jemalloc crate wasn't built.

While this seems somewhat reasonable as a niche situation, the same mechanism is
used for disabling jemalloc for platforms that just don't support it. For
example if the rumprun target is compiled then the sibiling Linux target *also*
doesn't have jemalloc. This is currently a problem for our cross-build nightlies
which build many targets. If rumprun is also built, it will disable jemalloc for
all targets, which isn't desired.

This commit moves the platform-specific disabling of jemalloc as hardcoded logic
into the makefiles that is scoped per-platform. This way when configuring
multiple targets **without the `--disable-jemalloc` option specified** all
targets will get jemalloc as they should.
@alexcrichton
Copy link
Member Author

@bors: r=brson b980f22

@bors
Copy link
Contributor

bors commented Feb 26, 2016

⌛ Testing commit b980f22 with merge 7bfdb5a...

@bors
Copy link
Contributor

bors commented Feb 26, 2016

⛄ The build was interrupted to prioritize another pull request.

bors added a commit that referenced this pull request Feb 26, 2016
The `--disable-jemalloc` configure option has a failure mode where it will
create a distribution that is not compatible with other compilers. For example
the nightly for Linux will assume that it will link to jemalloc by default as
an allocator for executable crates. If, however, a standard library is used
which was built via `./configure --disable-jemalloc` then this will fail
because the jemalloc crate wasn't built.

While this seems somewhat reasonable as a niche situation, the same mechanism is
used for disabling jemalloc for platforms that just don't support it. For
example if the rumprun target is compiled then the sibiling Linux target *also*
doesn't have jemalloc. This is currently a problem for our cross-build nightlies
which build many targets. If rumprun is also built, it will disable jemalloc for
all targets, which isn't desired.

This commit moves the platform-specific disabling of jemalloc as hardcoded logic
into the makefiles that is scoped per-platform. This way when configuring
multiple targets **without the `--disable-jemalloc` option specified** all
targets will get jemalloc as they should.
@bors
Copy link
Contributor

bors commented Feb 26, 2016

⌛ Testing commit b980f22 with merge f59fd46...

@bors bors merged commit b980f22 into rust-lang:master Feb 26, 2016
This was referenced Feb 26, 2016
@alexcrichton alexcrichton deleted the better-disable-jemallc branch March 1, 2016 00:38
@MagaTailor
Copy link

@alexcrichton What about --disable-jemalloc as a means to default to alloc_system but still building the crate on all platforms that support it?

Surely, the idea is not new, and @brson seems to like it - are there any caveats?

@alexcrichton
Copy link
Member Author

To me --disable-jemalloc means "turn of jemalloc as much as possible", but if we're switching the default allocator on some platforms then we could just change that in the compiler rather than the build system

@MagaTailor
Copy link

Ok, how would you go about changing the default allocator permanently, leaving the jemalloc option on the table?

@alexcrichton
Copy link
Member Author

By changing this line in the relevant target.

@MagaTailor
Copy link

Thanks for the suggestion - what about a more user-friendly configure switch?

@alexcrichton
Copy link
Member Author

I was under the impression we don't want it as a configure switch? If jemalloc is inferior on some platforms we should just always turn it off by default.

@MagaTailor
Copy link

MagaTailor commented Jun 30, 2016

Jemalloc is highly tunable so having the crate at your disposal makes sense, even though it's turned off by default. Right now, this scenario is not easily bootstrappable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants