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

Make incremental compilation the default for all profiles. #6564

Merged
merged 1 commit into from Jan 24, 2019

Conversation

Projects
None yet
8 participants
@michaelwoerister
Copy link
Contributor

michaelwoerister commented Jan 18, 2019

This PR makes incremental compilation the default for all profiles, that is, also release and bench. rustc performs ThinLTO by default for incremental release builds for a while now and the data we've gathered so far indicates that the generated binaries exhibit roughly the same runtime performance as non-incrementally compiled ones. At the same time, incremental release builds can be 2-5 times as fast as non-incremental ones.

Making incremental compilation (temporarily) the default in cargo would be a simple way of gathering more data about runtime performance via lolbench.rs. If the results look acceptable, we can just leave it on and give a massive compile time reduction to everyone. If not, we can revert the change and think about a plan B.

This strategy assumes that lolbench will actually use the nightly cargo version. Is that true, @anp?

r? @alexcrichton

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Jan 18, 2019

Interesting!

How does one enable incremental compilation for the release and bench profiles right now?

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Jan 18, 2019

By setting the CARGO_INCREMENTAL flag as in:

CARGO_INCREMENTAL=1 cargo build --release
@anp

This comment has been minimized.

Copy link
Member

anp commented Jan 18, 2019

lolbench consumes toolchains via rustup, so as long as that works for you you'll get the data you want. Gosh, I'm a little sad that not having had time for a try feature could now be a part of the geology of master :P.

EDIT: I think rust-lang/rust-central-station#82 (comment) will be a prereq of try-jobs if someone wants to help me out there.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 18, 2019

I'm personally fine with this, but this may want some more buy-in perhaps? Should we pull in the compiler team and/or FCP with a set of folks for this?

@ehuss ehuss referenced this pull request Jan 22, 2019

Open

Add build profile. #6577

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Jan 24, 2019

We discussed this in the @rust-lang/compiler triage meeting. The team seems to be in favor of enabling this now, in order to collect data via lolbench, and then unconditionally disabling it again at the beginning of the all hands.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 24, 2019

@bors: r+

Ok!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 24, 2019

📌 Commit 2c40858 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 24, 2019

⌛️ Testing commit 2c40858 with merge d5109ad...

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

Auto merge of #6564 - michaelwoerister:default_incremental_everywhere…
…, r=alexcrichton

Make incremental compilation the default for all profiles.

This PR makes incremental compilation the default for all profiles, that is, also `release` and `bench`. `rustc` performs ThinLTO by default for incremental release builds for a while now and the [data we've gathered so far](rust-lang/rust#56678) indicates that the generated binaries exhibit roughly the same runtime performance as non-incrementally compiled ones. At the same time, incremental release builds can be 2-5 times as fast as non-incremental ones.

Making incremental compilation (temporarily) the default in `cargo` would be a simple way of gathering more data about runtime performance via [lolbench.rs](https://lolbench.rs). If the results look acceptable, we can just leave it on and give a massive compile time reduction to everyone. If not, we can revert the change and think about a plan B.

This strategy assumes that lolbench will actually use the nightly `cargo` version. Is that true, @anp?

r? @alexcrichton
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 24, 2019

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

@bors bors merged commit 2c40858 into rust-lang:master Jan 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

bors added a commit to rust-lang/rust that referenced this pull request Jan 25, 2019

Auto merge of #57891 - michaelwoerister:no-default-incr-bootstrap, r=…
…pietroalbini

bootstrap: Don't rely on any default settings regarding incr. comp. in Cargo

rust-lang/cargo#6564 (temporarily) makes incremental compilation the default for release builds. We don't want this default to apply to building the compiler itself, that is, `bootstrap`'s `incremental` flag should always be respected. Otherwise we'll get incrementally build releases, which we really don't want `:)`.

r? @Mark-Simulacrum

Anybody else from @rust-lang/release should feel free to r+ this too if they get around to it earlier.
@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jan 28, 2019

I think this change broke release builds for embedded projects. The embedded template used by the community has this in its Cargo.toml:

[profile.release]
codegen-units = 1 # better optimizations
debug = true # symbols are nice and they don't increase the size on Flash
lto = true # better optimizations

These settings produce a the following error: "can't perform LTO when compiling incrementally" with the latest nightly. Adding incremental = false to Cargo.toml fixes the problem.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Jan 28, 2019

@japaric Since we are enabling this just for data collection at the moment and will revert at the end of this week, do think we should do something here now?

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jan 28, 2019

@michaelwoerister would it be possible to not enable incremental if lto is set to true in the Cargo.toml? I think that would avoid the breakage. If that's too much trouble then we'll just have to deal with the breakage reports until the weekend.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 28, 2019

I've gone ahead and posted the revert for this at #6610, and @michaelwoerister we've got a nightly or two now with the incremental bit turned on so I think we're good to go for benchmarking, right?

@japaric this is definitely something we'll handle before turning it back on by default for real!

@michaelwoerister do you perhaps want to make a tracking issue for turning on incremental by default in release mode?

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Jan 28, 2019

Which is the first nightly when this was on? (for looking at lolbench)

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 28, 2019

@Eh2406 nightly-2019-01-28 is the first one.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Jan 29, 2019

Sorry for the breakage, @japaric!

@michaelwoerister do you perhaps want to make a tracking issue for turning on incremental by default in release mode?

Will do.

If I understand things correctly #6610 reverts the change in Cargo but the change still takes some time to bubble up to nightlies? I think we have some good numbers already, so I'm OK with ending the experiment.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Jan 29, 2019

@michaelwoerister do you perhaps want to make a tracking issue for turning on incremental by default in release mode?

Done: rust-lang/rust#57968

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Jan 29, 2019

the change still takes some time to bubble up to nightlies?

Lately I've been doing updates around once a week. The last update was 2 days ago, so I would probably do it this coming weekend. Do you feel like this issue is important enough to update now? (Which is totally fine, it's usually not difficult, just depends on how long you want the experiment to last.)

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

michaelwoerister commented Jan 29, 2019

The original plan was to keep it running throughout the week. It depends on how much trouble this creates for the embedded folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment