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

Enable incremental by default #4817

Merged
merged 1 commit into from Dec 21, 2017

Conversation

Projects
None yet
7 participants
@alexcrichton
Member

alexcrichton commented Dec 13, 2017

This commit enables incremental compilation by default in Cargo for all
dev-related profiles (aka anything without --release or bench. A
number of new configuration options were also added to tweak how
incremental compilation is exposed and/or used:

  • A profile.dev.incremental field is added to Cargo.toml to disable
    it on a per-project basis (in case of bugs).
  • A build.incremental field was added in .cargo/config to disable
    globally (or enable if we flip this default back off).

Otherwise CARGO_INCREMENTAL can still be used to configure one
particular compilation. The global build.incremental configuration
cannot currently be used to enable it for the release profile.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 13, 2017

r? @matklad

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

rust-highfive commented Dec 13, 2017

r? @matklad

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 13, 2017

Member

Note that this is intended to land roughly the same time we rename -Z to -C for the incremental option but I haven't done that quite just yet to ensure that this still passes CI. We'll probably want to hold off on landing until that's ready to go on the rust-lang/rust side of things.

cc @michaelwoerister @nikomatsakis

Member

alexcrichton commented Dec 13, 2017

Note that this is intended to land roughly the same time we rename -Z to -C for the incremental option but I haven't done that quite just yet to ensure that this still passes CI. We'll probably want to hold off on landing until that's ready to go on the rust-lang/rust side of things.

cc @michaelwoerister @nikomatsakis

@matklad

Let's add tests for profile and .cargo/config configuration?

A question I'd like to ask is where this option indeed belongs to profile? My understanding here is that it indeed does, because incremental compilation could change the resulting artifact (i.e, LLVM may make different optimization decisions because of incremental).

Also, 🎉 🎊 🎆 🌠 🍒 🎉 🍕 🍍 !!!

Show outdated Hide outdated src/cargo/ops/cargo_rustc/context.rs Outdated
Show outdated Hide outdated src/cargo/ops/cargo_rustc/context.rs Outdated

@matklad matklad added the relnotes label Dec 13, 2017

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Dec 13, 2017

Member

Oh, we might want to check how this interacts with cargo check just in case something gets broken.

Member

matklad commented Dec 13, 2017

Oh, we might want to check how this interacts with cargo check just in case something gets broken.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 14, 2017

Member

@matklad

Let's add tests for profile and .cargo/config configuration?

Certainly! Should be done now.

A question I'd like to ask is where this option indeed belongs to profile?

Oh yeah incremental definitely affects compilation artifacts so we need to recompile whether it's on or off.

Member

alexcrichton commented Dec 14, 2017

@matklad

Let's add tests for profile and .cargo/config configuration?

Certainly! Should be done now.

A question I'd like to ask is where this option indeed belongs to profile?

Oh yeah incremental definitely affects compilation artifacts so we need to recompile whether it's on or off.

// all builds though. For example a `true` value doesn't enable
// release incremental builds, only dev incremental builds. This can
// be useful to globally disable incremental compilation like
// `CARGO_INCREMENTAL`.

This comment has been minimized.

@matklad

matklad Dec 14, 2017

Member

Hm, so the value true for build.incremental does not change anything, because true && default is just default.

So, the only usage for this flag is to disable incremental compilation. So perhaps we can rename the flag to build.disable_incremental and allow only true value for it?

Should this setting override even explicit profile incremental setting? If the primary use-case here is to work-around the possible bugs in incremental compilation, then I think a reliable way to turn off all incremental would be more valuable.

@matklad

matklad Dec 14, 2017

Member

Hm, so the value true for build.incremental does not change anything, because true && default is just default.

So, the only usage for this flag is to disable incremental compilation. So perhaps we can rename the flag to build.disable_incremental and allow only true value for it?

Should this setting override even explicit profile incremental setting? If the primary use-case here is to work-around the possible bugs in incremental compilation, then I think a reliable way to turn off all incremental would be more valuable.

This comment has been minimized.

@alexcrichton

alexcrichton Dec 14, 2017

Member

Yeah this was mostly just meant to mirror CARGO_INCREMENTAL as a "global configuration option" to disable it everywhere (I don't think enabling it everywhere makes much sense for release profiles for example).

I personally prefer incremental at least though to disable-incremental in that it's a bit more clear (even if incremental = true doesn't actually do anything).

I think that makes sense as well yeah for .cargo/config to override profiles, I'll tweak that

@alexcrichton

alexcrichton Dec 14, 2017

Member

Yeah this was mostly just meant to mirror CARGO_INCREMENTAL as a "global configuration option" to disable it everywhere (I don't think enabling it everywhere makes much sense for release profiles for example).

I personally prefer incremental at least though to disable-incremental in that it's a bit more clear (even if incremental = true doesn't actually do anything).

I think that makes sense as well yeah for .cargo/config to override profiles, I'll tweak that

@lilianmoraru

This comment has been minimized.

Show comment
Hide comment
@lilianmoraru

lilianmoraru Dec 18, 2017

So, the only usage for this flag is to disable incremental compilation. So perhaps we can rename the flag to build.disable_incremental and allow only true value for it?

From a user's perspective(my personal perspective) that doesn't know what the code does behind, incremental just seems like the more obvious option that would enable or disable incremental compilation and of course, it is more in line with CARGO_INCREMENTAL.

lilianmoraru commented Dec 18, 2017

So, the only usage for this flag is to disable incremental compilation. So perhaps we can rename the flag to build.disable_incremental and allow only true value for it?

From a user's perspective(my personal perspective) that doesn't know what the code does behind, incremental just seems like the more obvious option that would enable or disable incremental compilation and of course, it is more in line with CARGO_INCREMENTAL.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 21, 2017

Member

Ok now that we've got a -C option upstream, re-r? @matklad

Member

alexcrichton commented Dec 21, 2017

Ok now that we've got a -C option upstream, re-r? @matklad

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Dec 21, 2017

Member

@alexcrichton should we add some docs here?

Member

matklad commented Dec 21, 2017

@alexcrichton should we add some docs here?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 21, 2017

Member

Seems plausible yeah, I've added some docs for the config key, env var, and profile option. I don't think we'll want to highlight it too much though (aka explain at length what the defaults are) as hopefully everyone's just going to see faster compiles :)

Member

alexcrichton commented Dec 21, 2017

Seems plausible yeah, I've added some docs for the config key, env var, and profile option. I don't think we'll want to highlight it too much though (aka explain at length what the defaults are) as hopefully everyone's just going to see faster compiles :)

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Dec 21, 2017

Member

@bors r!

Excellent! 🚀

Member

matklad commented Dec 21, 2017

@bors r!

Excellent! 🚀

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Dec 21, 2017

Member

@bors r+

Robots are too unemotional :(

Member

matklad commented Dec 21, 2017

@bors r+

Robots are too unemotional :(

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

📌 Commit 475fa58 has been approved by matklad

Contributor

bors commented Dec 21, 2017

📌 Commit 475fa58 has been approved by matklad

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

⌛️ Testing commit 475fa58 with merge 203edea...

Contributor

bors commented Dec 21, 2017

⌛️ Testing commit 475fa58 with merge 203edea...

bors added a commit that referenced this pull request Dec 21, 2017

Auto merge of #4817 - alexcrichton:incremental-by-default, r=matklad
Enable incremental by default

This commit enables incremental compilation by default in Cargo for all
dev-related profiles (aka anything without `--release` or `bench`. A
number of new configuration options were also added to tweak how
incremental compilation is exposed and/or used:

* A `profile.dev.incremental` field is added to `Cargo.toml` to disable
  it on a per-project basis (in case of bugs).
* A `build.incremental` field was added in `.cargo/config` to disable
  globally (or enable if we flip this default back off).

Otherwise `CARGO_INCREMENTAL` can still be used to configure one
particular compilation. The global `build.incremental` configuration
cannot currently be used to enable it for the release profile.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Dec 21, 2017

💔 Test failed - status-travis

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad
Member

matklad commented Dec 21, 2017

Enable incremental by default
This commit enables incremental compilation by default in Cargo for all
dev-related profiles (aka anything without `--release` or `bench`. A
number of new configuration options were also added to tweak how
incremental compilation is exposed and/or used:

* A `profile.dev.incremental` field is added to `Cargo.toml` to disable
  it on a per-project basis (in case of bugs).
* A `build.incremental` field was added in `.cargo/config` to disable
  globally (or enable if we flip this default back off).

Otherwise `CARGO_INCREMENTAL` can still be used to configure one
particular compilation. The global `build.incremental` configuration
cannot currently be used to enable it for the release profile.
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Dec 21, 2017

Member

@bors: r=matklad

Member

alexcrichton commented Dec 21, 2017

@bors: r=matklad

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

📌 Commit 45cc30b has been approved by matklad

Contributor

bors commented Dec 21, 2017

📌 Commit 45cc30b has been approved by matklad

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

⌛️ Testing commit 45cc30b with merge e10c651...

Contributor

bors commented Dec 21, 2017

⌛️ Testing commit 45cc30b with merge e10c651...

bors added a commit that referenced this pull request Dec 21, 2017

Auto merge of #4817 - alexcrichton:incremental-by-default, r=matklad
Enable incremental by default

This commit enables incremental compilation by default in Cargo for all
dev-related profiles (aka anything without `--release` or `bench`. A
number of new configuration options were also added to tweak how
incremental compilation is exposed and/or used:

* A `profile.dev.incremental` field is added to `Cargo.toml` to disable
  it on a per-project basis (in case of bugs).
* A `build.incremental` field was added in `.cargo/config` to disable
  globally (or enable if we flip this default back off).

Otherwise `CARGO_INCREMENTAL` can still be used to configure one
particular compilation. The global `build.incremental` configuration
cannot currently be used to enable it for the release profile.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing e10c651 to master...

Contributor

bors commented Dec 21, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing e10c651 to master...

@bors bors merged commit 45cc30b into rust-lang:master Dec 21, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@SimonSapin SimonSapin referenced this pull request Jan 4, 2018

Open

Per-target profiles? #4897

@aidanhs

This comment has been minimized.

Show comment
Hide comment
@aidanhs

aidanhs Jan 9, 2018

Member

As a point of interest, some (single binary) projects I'm looking at see a ~10% increase in the size of the target directory to store incremental things (~150MB in incremental dir, ~1.1GB overall).

Projects with more binaries will presumably see more impact, and things like crater (which build every single crate individually) see approximately a doubling of target dir size.

Member

aidanhs commented Jan 9, 2018

As a point of interest, some (single binary) projects I'm looking at see a ~10% increase in the size of the target directory to store incremental things (~150MB in incremental dir, ~1.1GB overall).

Projects with more binaries will presumably see more impact, and things like crater (which build every single crate individually) see approximately a doubling of target dir size.

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jan 10, 2018

Contributor

Good to know, @aidanhs! A doubling of size is to be expected since we are basically keeping around a second version of the program in the incr. comp. cache. If it were much more than double, then that would be an indicator that cache garbage collection doesn't work as expected.

Contributor

michaelwoerister commented Jan 10, 2018

Good to know, @aidanhs! A doubling of size is to be expected since we are basically keeping around a second version of the program in the incr. comp. cache. If it were much more than double, then that would be an indicator that cache garbage collection doesn't work as expected.

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