Closed Bug 1436251 Opened 2 years ago Closed 2 years ago

Set codegen-units=1 in --enable-release

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

Details

Attachments

(1 file)

We currently use the default codegen-units (16) in --enable-release mode. codegen-units gives us parallelism during the build in exchange for losing some optimization opportunities.

In --enable-release, we don't want to lose optimization opportunities.

Furthermore, for some reason, codegen-units=1 makes the --enable-release build *faster* (cuts time taken by half):


For an --enable-debug --enable-optimize --enable-release build with -j8:

with codegen-units=1
real    20m35.218s
user    135m46.044s
sys     5m49.332s

with default codegen-units (16)
real    38m3.751s
user    156m53.384s
sys     6m22.100s


I'm not sure why; codegen-units=16 should be faster to compile, but it somehow isn't. I tried to look into it and couldn't see any immediately obvious reason. (It's worth verifying this on other machines though)


The codegen-units=1 build was also 1MB smaller. So it seems to be a clear win, and we should try using codegen-units=1 for all --enable-release builds.
Comment on attachment 8948882 [details]
Bug 1436251 - Set codegen-units=1 in --enable-release;

https://reviewboard.mozilla.org/r/218280/#review224062
Attachment #8948882 - Flags: review+
Attachment #8948882 - Flags: review?(nfroyd)
Assignee: nobody → manishearth
Comment on attachment 8948882 [details]
Bug 1436251 - Set codegen-units=1 in --enable-release;

https://reviewboard.mozilla.org/r/218280/#review224072
Attachment #8948882 - Flags: review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fe90c7d53a7c
Set codegen-units=1 in --enable-release; r=glandium
https://hg.mozilla.org/mozilla-central/rev/fe90c7d53a7c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
We didn't receive any Perfherder alerts for this change. Given the claims in comment #0, I would expect an alert for at least the installer size. Due to sccache and cache hits and normal build time variance, I would not be surprised to not see an alert for build times.

Is it worth digging through data to see why nothing apparently changed/alerted?
Yeah. I locally got major differences on both Linux and my Macbook.
Does codegen-units default universally to 16, or does it depend on the number of cores detected? That might differ significantly between local builds and automation.
Well, before codegen-units wasn't set and would presumably default to whatever `cargo` decided. We almost always build with 16 vCPU instances (advertise 16 cores) in CI.
(relatedly, it seems to me rustc should use the make job server to start up its codegen units)
I just measured locally on my ThinkStation with/without Manish's patch reverted.

Vanilla:  23:48 clobber, 11:14 incremental, 92993158 text segment
Reverted: 23:45 clobber, 11:03 incremental, 92992982 text segment

This is with rustc 1.23.0 (766bd11c8 2018-01-01)

So Manish's patch does not appear to have had an effect on my machine. Next step is for Manish to try reverting his patch and comparing the results on his machine where he observed the original difference.
Flags: needinfo?(manishearth)
Oh, I know what's going on.

I've been building with Nightly.

https://github.com/rust-lang/rust/pull/46910 (and previous changes setting the default to 16) affects Nightly, but has not yet reached stable.

So this change keeps stuff fast for people using nightly locally.

Given that we only guarantee that Firefox will build with our recommended stable rustc, this change isn't strictly necessary. I'm not sure how many people build Firefox with nightly Rust, if it's very few we should just revert this.
Flags: needinfo?(manishearth)
(In reply to Manish Goregaokar [:manishearth] from comment #12)
> Given that we only guarantee that Firefox will build with our recommended
> stable rustc, this change isn't strictly necessary. I'm not sure how many
> people build Firefox with nightly Rust, if it's very few we should just
> revert this.

Is the intention for those changes to ride the trains? If so, seems like we'll want this soon in any case.
Yes. The fact that codegen-units=16 affects compile times *negatively* (especially this severely) is a bug IMO, and we should perhaps try and investigate that.

The increased size due to codegen-units=16 is to be expected, though it is a bit more than I would have guessed, and I'd expect LTO to be able to deal with that at least a bit?
(In reply to Mike Hommey [:glandium] from comment #10)
> (relatedly, it seems to me rustc should use the make job server to start up
> its codegen units)

This is already the case, FYI:
https://github.com/rust-lang/rust/blob/29c8276cee4a0eab7e0634ff25c6b47bd9f87c6c/src/librustc/session/mod.rs#L811
Filed https://github.com/rust-lang/rust/issues/48233 as well

The Rust release at the end of this week will contain the default CU changes
Looks like https://github.com/rust-lang/rust/pull/48163 may fix this issue. I'll try it out and report back.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.