Closed Bug 1437627 Opened 2 years ago Closed 2 years ago

enable incremental compilation for Rust by default

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(3 files, 2 obsolete files)

We can't/shouldn't do this for automation builds (see bug 1430886 comments 21 and 25), but there's no particular reason that we couldn't turn it on for developer builds.

I guess this technically doesn't depend on requiring Rust 1.24 (bug 1430927)--people with Rust betas (and even release versions?) would get incremental compilation if we turned on CARGO_INCREMENTAL=1--but I think we'd feel better about enabling it only when the Rust team has decided it's ready for general use.
Product: Core → Firefox Build System
Not sure if this is going to wreak havoc with people trying to do performance
profiling or similar.  We should discuss that...
Attachment #8956884 - Flags: review?(core-build-config-reviews)
Comment on attachment 8956884 [details] [diff] [review]
enable Rust incremental compilation by default for non-debug builds

Review of attachment 8956884 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ +869,5 @@
>  ifdef MOZ_AUTOMATION
>  cargo_incremental := CARGO_INCREMENTAL=0
> +else
> +# Incremental compilation is enabled by default for debug builds, but we need
> +# to explicitly enable it for release builds.

Is this comment wrong? The patch and commit message point to the opposite.
(In reply to Chris Manchester (:chmanchester) from comment #2)
> Comment on attachment 8956884 [details] [diff] [review]
> enable Rust incremental compilation by default for non-debug builds
> 
> Review of attachment 8956884 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/rules.mk
> @@ +869,5 @@
> >  ifdef MOZ_AUTOMATION
> >  cargo_incremental := CARGO_INCREMENTAL=0
> > +else
> > +# Incremental compilation is enabled by default for debug builds, but we need
> > +# to explicitly enable it for release builds.
> 
> Is this comment wrong? The patch and commit message point to the opposite.

The condition is `ifndef MOZ_DEBUG_RUST`, which is the same condition that we use to enable --release for `cargo build`.  So I don't think it is?  Maybe the commit message should explicitly say "release" instead of "non-debug"?
(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Chris Manchester (:chmanchester) from comment #2)
> > Comment on attachment 8956884 [details] [diff] [review]
> > enable Rust incremental compilation by default for non-debug builds
> > 
> > Review of attachment 8956884 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: config/rules.mk
> > @@ +869,5 @@
> > >  ifdef MOZ_AUTOMATION
> > >  cargo_incremental := CARGO_INCREMENTAL=0
> > > +else
> > > +# Incremental compilation is enabled by default for debug builds, but we need
> > > +# to explicitly enable it for release builds.
> > 
> > Is this comment wrong? The patch and commit message point to the opposite.
> 
> The condition is `ifndef MOZ_DEBUG_RUST`, which is the same condition that
> we use to enable --release for `cargo build`.  So I don't think it is? 
> Maybe the commit message should explicitly say "release" instead of
> "non-debug"?

Oh, I misread this comment as "Incremental compilation is enabled by default [with the following code] for debug builds", not "we need to explicitly enable it [with the following code]...".
(In reply to Nathan Froyd [:froydnj] from comment #1)
> Created attachment 8956884 [details] [diff] [review]
> enable Rust incremental compilation by default for non-debug builds
> 
> Not sure if this is going to wreak havoc with people trying to do performance
> profiling or similar.  We should discuss that...

Incremental builds have worse runtime performance, 2x-3x slower is not uncommon, especially in micro benchmarks. However, if that is fast enough, it should speed up compilation considerably.
Let me know if I can help!
Realized that we shouldn't enable this for local --enable-release builds, as
incremental compilation would affect performance too much.
Attachment #8957684 - Flags: review?(core-build-config-reviews)
Attachment #8956884 - Attachment is obsolete: true
Attachment #8956884 - Flags: review?(core-build-config-reviews)
We want to use this to compute whether incremental compilation can be
used for Rust, so we need access to it outside of rust_compiler_flags.
Attachment #8957721 - Flags: review?(core-build-config-reviews)
Attachment #8957684 - Attachment is obsolete: true
Attachment #8957684 - Flags: review?(core-build-config-reviews)
Doing checks in Python is much simpler than doing them in Makefile
logic, and we're going to be adding some more complex logic in the next patch.
Attachment #8957722 - Flags: review?(core-build-config-reviews)
Attachment #8957723 - Flags: review?(core-build-config-reviews)
Comment on attachment 8957721 [details] [diff] [review]
part 1 - separate out a function for rustc's opt level

Review of attachment 8957721 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/toolchain.configure
@@ +1425,2 @@
>                          moz_optimize):
>      optimize = moz_optimize.optimize

This local "optimize" and the dependency on moz_optimize are no longer used here.
Attachment #8957721 - Flags: review?(core-build-config-reviews) → review+
Attachment #8957722 - Flags: review?(core-build-config-reviews) → review+
Attachment #8957723 - Flags: review?(core-build-config-reviews) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2c1f3ce833
part 1 - separate out a function for rustc's opt level; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/10df4610434b
part 2 - move incremental compilation logic to moz.configure; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/18315f44fad7
part 3 - enable incremental compilation for Rust; r=chmanchester
You need to log in before you can comment on or make changes to this bug.