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.

Sign up for GitHub

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

Jump to bottom

Do not enable ThinLTO on stable, beta, or nightly builds. #47834

Merged
merged 1 commit into from
Feb 4, 2018
Merged

Do not enable ThinLTO on stable, beta, or nightly builds. #47834

merged 1 commit into from
Feb 4, 2018

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jan 28, 2018 •
edited
Loading

Fixes #45444

@rust-highfive rust-highfive assigned aturon and unassigned alexcrichton Jan 28, 2018
@rust-highfive
Copy link
Contributor

r? @aturon

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

@Mark-Simulacrum
Copy link
Member Author

Not sure if we want to disable this for nightly builds, but the other two I feel fairly confident about.

@Mark-Simulacrum
Copy link
Member Author

r? @alexcrichton

@alexcrichton
Copy link
Member

Thanks! I think we want to keep this enabled though for the rest of CI? I think as-is it'd blanket turn it off on Travis/AppVeyor?

@Mark-Simulacrum
Copy link
Member Author

So keep ThinLTO on for nightly but not beta and stable? I am okay with that.

@alexcrichton
Copy link
Member

Hm actually, how about an off-by-default option enabled here which is our area for "produce a release config flags to pass" stuff

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2018
@Mark-Simulacrum Mark-Simulacrum added this to the 1.24 milestone Jan 31, 2018
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2018
@Mark-Simulacrum Mark-Simulacrum force-pushed the no-cgu-release branch 2 times, most recently from 7301e14 to bbb9860 Compare February 2, 2018 18:45
@Mark-Simulacrum
Copy link
Member Author

@bors try -- to test and make sure that nothing breaks on dist builds

@bors
Copy link
Collaborator

bors commented Feb 2, 2018

⌛ Trying commit bbb9860 with merge d2192a1...

bors added a commit that referenced this pull request Feb 2, 2018
@bors
Auto merge of #47834 - Mark-Simulacrum:no-cgu-release, r=<try>
d2192a1 
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
Copy link
Collaborator

bors commented Feb 2, 2018

💔 Test failed - status-travis

@ollie27
Copy link
Member

ollie27 commented Feb 2, 2018

Won't you need to specify 1 codegen unit as rustc defaults to 16 now or am I missing something?

@kennytm
Copy link
Member

kennytm commented Feb 3, 2018

@bors try

@bors
Copy link
Collaborator

bors commented Feb 3, 2018

⌛ Trying commit 08159e9 with merge 20e1b39...

bors added a commit that referenced this pull request Feb 3, 2018
@bors
Auto merge of #47834 - Mark-Simulacrum:no-cgu-release, r=<try>
20e1b39 
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
Copy link
Collaborator

bors commented Feb 3, 2018

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

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2018
@Mark-Simulacrum
Copy link
Member Author

@alexcrichton Ready for another review.

alexcrichton
alexcrichton reviewed Feb 3, 2018
View reviewed changes
src/bootstrap/config.rs Outdated
@@ -466,6 +468,7 @@ impl Config {
set(&mut config.quiet_tests, rust.quiet_tests);
set(&mut config.test_miri, rust.test_miri);
set(&mut config.wasm_syscall, rust.wasm_syscall);
config.rust_thinlto = rust.thinlto.unwrap_or(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this section will only get hit if rust is Some (which it may not be), so could the default-true initialization happen sooner?

@alexcrichton
Copy link
Member

I think due to the change in defaults in rustc this ay also not work? When thinlto is disabled I think we'll want to basically pass -C codegen-units=1 to rustc, right?

@Mark-Simulacrum
Disable ThinLTO for dist builds.
e1f04c0 
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
dist builders.
@Mark-Simulacrum
Copy link
Member Author

Updated to pass the 1-codegen-unit option to rustc.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 4, 2018

📌 Commit e1f04c0 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 4, 2018

⌛ Testing commit e1f04c0 with merge 0c6091f...

bors added a commit that referenced this pull request Feb 4, 2018
@bors
Auto merge of #47834 - Mark-Simulacrum:no-cgu-release, r=alexcrichton
0c6091f 
Do not enable ThinLTO on stable, beta, or nightly builds.

Fixes #45444
@bors
Copy link
Collaborator

bors commented Feb 4, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0c6091f to master...

@bors bors merged commit e1f04c0 into rust-lang:master Feb 4, 2018
@aidanhs aidanhs mentioned this pull request Feb 5, 2018
Closed
@tjkirch tjkirch mentioned this pull request Mar 15, 2018
Closed
@Mark-Simulacrum Mark-Simulacrum deleted the no-cgu-release branch June 8, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
1.24
Development

Successfully merging this pull request may close these issues.
7 participants
@Mark-Simulacrum @rust-highfive @alexcrichton @bors @ollie27 @kennytm @aturon