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.

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

codegen-units=16 doubles compile times for Firefox #48233

Closed
Manishearth opened this issue Feb 15, 2018 · 16 comments
Closed

codegen-units=16 doubles compile times for Firefox #48233

Manishearth opened this issue Feb 15, 2018 · 16 comments
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

https://bugzilla.mozilla.org/show_bug.cgi?id=1436251

The other day I was experimenting with codegen-units, and realized that Firefox's build gets really slow with codgen-units=16 (the default):

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 wasn't able to reduce the testcase.

If you wish to do it, use nightly to build Firefox with the following mozconfig file:

ac_add_options --enable-debug
ac_add_options --enable-optimize
ac_add_options --enable-release

and revert this patch: https://hg.mozilla.org/mozilla-central/rev/fe90c7d53a7c

A useful bit of information may be that we pass -Clto only to the toplevel crate, i.e. we build with cargo rustc -- -Clto. I don't remember but it might have been only the last crate ("gkrust") taking forever to build (I can retest this if you wish) so it might be some pathological case with LTO.

cc @michaelwoerister

@Manishearth Manishearth added A-codegen Area: Code generation I-compiletime Issue: Problems and improvements with respect to compile times. labels Feb 15, 2018
@matthiaskrgr
Copy link
Member

Sounds like #48025

@Manishearth
Copy link
Member Author

cc @alexcrichton does this sound like a dupe of #48163 ?

I'm going to test with/without that PR soon.

@michaelwoerister
Copy link
Member

Depending on what Rust version you are using, it might also be related to #48226?

@michaelwoerister
Copy link
Member

michaelwoerister commented Feb 15, 2018

What's the compile time without the ac_add_options --enable-debug setting?

@Manishearth
Copy link
Member Author

Manishearth commented Feb 15, 2018 via email

@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 15, 2018
@nikomatsakis
Copy link
Contributor

Nominating for prioritization and discussion (presuming it's not resolved before then).

@jkordish jkordish added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Feb 21, 2018
@nikomatsakis
Copy link
Contributor

triage: P-medium

Marking as P-medium for now until we get some more heat from the FF folks. ;)

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Feb 22, 2018
@nikomatsakis
Copy link
Contributor

In particular, nobody is using CGU=16 yet, right? (But also, it's not entirely obvious to me that it would be desirable, given #47745)

@Manishearth
Copy link
Member Author

I tried it out again and it seems to be fixed.

CGU=16 is the default, yeah? So everyone is using it.

@michaelwoerister
Copy link
Member

@Manishearth, I thought that FF is compiled with codegen-units=4?

@Manishearth
Copy link
Member Author

Release mode uses the default.

@nikomatsakis
Copy link
Contributor

Mmmmm, that might bump the priority somewhat. =)

@michaelwoerister
Copy link
Member

Release mode uses the default.

Is this with full LTO?

@Manishearth
Copy link
Member Author

I'm not sure, eddyb said that LTO turns it off.

However, Firefox builds with LTO turned on only for the last crate.

@alexcrichton
Copy link
Member

To clarify, rustc is using 16 codegen units by default in all modes, regardless of optimization level, LTO settings, etc. @Manishearth you mention this is fixed now though? If that's the case this was indeed likely fixed by #48163

@Mark-Simulacrum
Copy link
Member

Based on "I tried it out again and it seems to be fixed." from #48233 (comment), I'm going to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants