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

rustbuild: Disable multiple CGUs in stable release builds #45444

Closed
petrochenkov opened this issue Oct 22, 2017 · 10 comments
Closed

rustbuild: Disable multiple CGUs in stable release builds #45444

petrochenkov opened this issue Oct 22, 2017 · 10 comments
Assignees
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. P-high High priority T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 22, 2017

#45400 enabled multiple CGUs + ThinLTO in rustc builds to speedup bootstrap, however it made the produced compiler slower (see #45400 (comment) and below).

This is desirable for development builds, where build times are priority, but not for stable release builds that happen rarely (so build times don't matter much) and produce binaries used by majority of Rust users.

Enabling full LTO (#45400 (comment)) in stable release builds is also worth investigating.

@petrochenkov petrochenkov added A-build T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 22, 2017
@Mark-Simulacrum
Copy link
Member

Responding to @michaelwoerister from this comment:

I agree that we should build stable and beta releases with just one CGU and whatever other performance optimization we can do. I think the only reason we don't use regular LTO for stable releases is that rustc is built out of Rust dylibs which are incompatible with LTO.

I'm personally not opposed to shipping faster stable/beta artifacts -- we already do this by disabling LLVM assertions for example, so I don't see why disabling ThinLTO is a bad thing here. Seems like a fairly clear win, at least until ThinLTO is no longer a loss.

@alexcrichton, @Mark-Simulacrum, do we have numbers on how this influences build times in CI?

Not that I know of. Travis is problematic due to running our builds always at least once extra and often more than that, but we may be able to get data from @aidanhs. I'd also be interested in relative performance increases locally (building the compiler itself) but that just needs someone to be patient enough to rebuild the compiler with and without this patch.

@michaelwoerister
Copy link
Member

I'm personally not opposed to shipping faster stable/beta artifacts -- we already do this by disabling LLVM assertions for example

That would have been my line of argument too.

@alexcrichton
Copy link
Member

Yes as @michaelwoerister mentioned the current design with dylibs inhibits full LTO (in general procedural macros throw a wrench into full LTO).

For disabling multiple CGUs/ThinLTO it seems fine to do that on release builds which as mentioned already have other tweaks anyway.

@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.22, 1.21 Oct 23, 2017
@aidanhs
Copy link
Member

aidanhs commented Oct 24, 2017

I might be able to get some stats if you let me know what you'd like to compare - before and after a particular commit I assume? There's a lot of noise in Travis (particularly at the moment) so there may not be very high quality results.

@Mark-Simulacrum Mark-Simulacrum added the P-high High priority label Nov 15, 2017
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.22, 1.23 Nov 15, 2017
@Mark-Simulacrum
Copy link
Member

Increasing priority to high as I'd prefer that beta didn't have this, but if it slips that doesn't seem to bad as we'll be fine for one more cycle.

@Mark-Simulacrum
Copy link
Member

@alexcrichton So if I'm correct at least the last stable release shipped with this? I don't think there's been any movement yet... I can probably make an implementation if we'd like to make beta/stable builds get built without ThinLTO. Is that the consensus?

@alexcrichton
Copy link
Member

@Mark-Simulacrum hm maybe? I'm not sure myself.

In any case seems fine to draft a patch!

@Mark-Simulacrum Mark-Simulacrum self-assigned this Jan 16, 2018
@Mark-Simulacrum
Copy link
Member

I'll assign myself and try to get a patch up soon.

@jkordish jkordish added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jan 24, 2018
@nikomatsakis
Copy link
Contributor

It's hard for me to get a sense of how big the regression actually is. @alexcrichton suggests pretty small? In any case, I do think it makes sense to make our artifacts as good as they can be.

@alexcrichton
Copy link
Member

@nikomatsakis IIRC yes it was ~5% at most in some cases

@Mark-Simulacrum Mark-Simulacrum removed this from the 1.23 milestone Jan 28, 2018
bors added a commit that referenced this issue Feb 2, 2018
Do not enable ThinLTO on stable, beta, or nightly builds.

Developers testing locally (-dev profile) may want faster builds
for slightly slower compilers (~5%) whereas dist builds should always be
as fast as we can make them, and since those run on CI we don't care
quite as much for the build being somewhat slower. As such, we don't
automatically enable ThinLTO on builds for the stable, beta, or nightly
channels.

Fixes #45444
bors added a commit that referenced this issue Feb 3, 2018
Do not enable ThinLTO on stable, beta, or nightly builds.

Developers testing locally (-dev profile) may want faster builds
for slightly slower compilers (~5%) whereas dist builds should always be
as fast as we can make them, and since those run on CI we don't care
quite as much for the build being somewhat slower. As such, we don't
automatically enable ThinLTO on builds for the stable, beta, or nightly
channels.

Fixes #45444
bors added a commit that referenced this issue Feb 4, 2018
Do not enable ThinLTO on stable, beta, or nightly builds.

Fixes #45444
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. P-high High priority T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants