Stream: t-compiler

Topic: #56812 - why is jemalloc linked to rustc only via CI?


gnzlbg (Dec 14 2018 at 12:28, on Zulip):

@pnkfelix so we probably want to just encode a platform-dependent default for jemalloc, but we can't do that in the configuration directly. So that PR just added an option off by default, and turned it on on CI for the platforms that support it.

gnzlbg (Dec 14 2018 at 12:29, on Zulip):

That changed the default for people building rustc themselves

pnkfelix (Dec 14 2018 at 12:29, on Zulip):

yeah I eventually realized that and tried to write that at the bottom of the issue description

gnzlbg (Dec 14 2018 at 12:30, on Zulip):

sounds like an oversight, encoding platform dependent defaults is not possible in the configuration

pnkfelix (Dec 14 2018 at 12:30, on Zulip):

in terms of saying "I had thought this option would be a no-op on other targets, but it appears that is not the case."

gnzlbg (Dec 14 2018 at 12:30, on Zulip):

we probably need some logic in whatever code parses the configuration file to, if the platform is x, and x supports jemalloc, and the user did not explicitly disable it, then turn it on

pnkfelix (Dec 14 2018 at 12:30, on Zulip):

sounds like an oversight, encoding platform dependent defaults is not possible in the configuration

As in: You are saying the oversight here is that the configuration should allow such encoding of platform-specific defaults?

gnzlbg (Dec 14 2018 at 12:31, on Zulip):

no, the configuration does not allow this, so nobody thought about doing that, it would be a lot of work to support this generally, for unclear wins

pnkfelix (Dec 14 2018 at 12:31, on Zulip):

I don't know if the logic needs to go into the config file (or into the config.toml file parsing). Couldn't we use #[cfg] switches in the rust code itself that refers to the jemalloc crate jemalloc_sys crate?

gnzlbg (Dec 14 2018 at 12:32, on Zulip):

I think global_allocator does not work for rustc

gnzlbg (Dec 14 2018 at 12:32, on Zulip):

so we can't use normal tools for this

pnkfelix (Dec 14 2018 at 12:32, on Zulip):

I'm not talking about global_allocator

pnkfelix (Dec 14 2018 at 12:32, on Zulip):

I'm talking about this line:

pnkfelix (Dec 14 2018 at 12:32, on Zulip):

https://github.com/rust-lang/rust/pull/55238/files#diff-707a0eda6b2f1a0537abc3d23133748cR67

pnkfelix (Dec 14 2018 at 12:32, on Zulip):

can't we add additional cfg switches to it, beyond the jemalloc-sys cfg that is already there, ensuring that it never gets turned on except on Linux or Mac OS X?

gnzlbg (Dec 14 2018 at 12:33, on Zulip):

maybe? it also works on freebsd, ... and other targets

gnzlbg (Dec 14 2018 at 12:33, on Zulip):

i think maybe we could use target dependent dependencies in the Cargo.toml ?

pnkfelix (Dec 14 2018 at 12:34, on Zulip):

hmmm. okay that (BSD support for the config flag) may be an argument for not doing it quite that way

gnzlbg (Dec 14 2018 at 12:34, on Zulip):

https://github.com/rust-lang/rust/pull/55238/files#diff-c525f98895ef98621ee01e8844f8b88f

gnzlbg (Dec 14 2018 at 12:34, on Zulip):

so jemalloc-sys is an optional dependency, toggled from somewhere else

gnzlbg (Dec 14 2018 at 12:35, on Zulip):

TBH the quickest fix would be to just change that toggle to "force enable/disable", and give it meaningful defaults somehow if its not present

pnkfelix (Dec 14 2018 at 12:35, on Zulip):

I would't be opposed to a ternary value there, sure.

gnzlbg (Dec 14 2018 at 12:36, on Zulip):

https://github.com/rust-lang/rust/pull/55238/files#diff-6710e3160b86e8cc371d2ff58ae1061cR534

gnzlbg (Dec 14 2018 at 12:36, on Zulip):

there is the code to modify, if the flag is not present, and we are in some platform where jemalloc should be use it by default, just do so

gnzlbg (Dec 14 2018 at 12:36, on Zulip):

maybe one day we can move that to Cargo.toml somehow properly

pnkfelix (Dec 14 2018 at 12:37, on Zulip):

Okay. Maybe we should transcribe a summary of this conversation to #56812

gnzlbg (Dec 14 2018 at 12:37, on Zulip):

yup

pnkfelix (Dec 14 2018 at 12:38, on Zulip):

(its also worth noting that some people may not agree with my premise that trying to make local build match CI is always right choice. See e.g. recent comment from @Nikita Popov on #56812 )

Last update: Nov 22 2019 at 04:35UTC