Closed Bug 1386371 Opened 8 years ago Closed 7 years ago

Disable Rust LTO by default in local builds

Categories

(Firefox Build System :: General, enhancement, P1)

Product:
Firefox Build System
Component:
General
Type:
enhancement

Tracking

(firefox54 unaffected, firefox55 unaffected, firefox56 wontfix, firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: cpeterson, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1386371 - Disable LTO by default, but enable in automation.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Rust LTO is very slow and cause significant build time regressions, but Rust LTO's run-time performance speedup is small. Michael Woerister says disabling Rust LTO "would basically eliminate the build time for gkrust" (the Stylo crate). We still want Rust LTO enabled in automation builds.
There's not a *great* way to do this, since Cargo only provides dev and release profiles, so we couldn't define our own profile that does LTO, but we could remove LTO from the release profile: https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/toolkit/library/rust/Cargo.toml#43 and then pass `-C lto` in RUSTFLAGS when we want it enabled: https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/config/rules.mk#865 I guess we can just make this something like `--enable-rust-lto`, and maybe default it to on for PGO builds?
On IRC, mbrubeck said we can't just add `-C lto` to RUSTFLAGS in mozconfig because `-C lto` is invalid when compiling Rust libraries. He suggested preprocessing (just) toolkit/library/rust/Cargo.toml to add the LTO RUSTFLAGS.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1) > I guess we can just make this something like `--enable-rust-lto`, and maybe > default it to on for PGO builds? This sounds ok to me, but in that case we need to make sure we do "PGO" builds on Mac as well. Turning off LTO seems to make stylo around 25% slower. This is ok for local builds, but we definitely want nightly / PGO builds to have LTO.
Tying this flag to --enable-release would do what we want; --enable-release turns on other optimizations like identical code folding and such, that are too expensive to do otherwise. (In reply to Chris Peterson [:cpeterson] from comment #2) > On IRC, mbrubeck said we can't just add `-C lto` to RUSTFLAGS in mozconfig > because `-C lto` is invalid when compiling Rust libraries. He suggested > preprocessing (just) toolkit/library/rust/Cargo.toml to add the LTO > RUSTFLAGS. Ew. I suppose there's no good way to specify flags for only the final link.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3) > This sounds ok to me, but in that case we need to make sure we do "PGO" > builds on Mac as well. Turning off LTO seems to make stylo around 25% > slower. This is ok for local builds, but we definitely want nightly / PGO > builds to have LTO. We could just add `--enable-rust-lto` to the Mac Nightly mozconfig, or do it for all opt mac automation builds if we're not unhappy with the build time.
I will take this, unless somebody else has been working on a patch in the interim. I filed https://github.com/rust-lang/cargo/issues/4349 for trying to communicate link-only flags, and Alex sounds very receptive to adding something and/or fixing issues in rustc itself. If something can get added soonish, we might even be able to uplift something for Rust 1.20?
Assignee: nobody → nfroyd
In the meantime, it seems like we should preprocess Cargo.toml. BTW, a cargo command line argument to override Cargo.toml attributes would go a long way too...
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3) > > This sounds ok to me, but in that case we need to make sure we do "PGO" > > builds on Mac as well. Turning off LTO seems to make stylo around 25% > > slower. This is ok for local builds, but we definitely want nightly / PGO > > builds to have LTO. > > We could just add `--enable-rust-lto` to the Mac Nightly mozconfig, or do it > for all opt mac automation builds if we're not unhappy with the build time. The advantage of this is that Talos would be an accurate reflection of what we ship. We should probably do this at least for now while we're trying to ship stylo. We can revisit it down the line if we want to speed up automation builds.
(In reply to Chris Peterson [:cpeterson] from comment #0) > Rust LTO is very slow and cause significant build time regressions, but Rust > LTO's run-time performance speedup is small. Michael Woerister says > disabling Rust LTO "would basically eliminate the build time for gkrust" > (the Stylo crate). > > We still want Rust LTO enabled in automation builds. To be clear, when I say it eliminates the build time for gkrust, I'm talking specifically about this measurement: https://github.com/rust-lang/rust/issues/43211#issuecomment-316080169 In other words, it is really only the gkrust crate that is affected -- while other crates like style or webrender are not affected by disabling LTO. It should still speed up things by at least 2 minutes.
(In reply to Michael Woerister from comment #9) > To be clear, when I say it eliminates the build time for gkrust, I'm talking > specifically about this measurement: > https://github.com/rust-lang/rust/issues/43211#issuecomment-316080169 > In other words, it is really only the gkrust crate that is affected -- while > other crates like style or webrender are not affected by disabling LTO. It > should still speed up things by at least 2 minutes. Does disabling LTO only help with gkrust build time because that is when the style, webrender, and geckoservo crates are actually linked?
(In reply to Chris Peterson [:cpeterson] from comment #10) > Does disabling LTO only help with gkrust build time because that is when the > style, webrender, and geckoservo crates are actually linked? Yes
Alex, is it possible to control whether LTO is enabled from Cargo command-line argument? If not, is that something that could be added to Cargo?
Flags: needinfo?(acrichton)
Ah yeah unfortunately Cargo doesn't currently have support for enabling/disabling LTO from the command line, but work is happening upstream to do so! -- https://github.com/rust-lang/cargo/issues/4349
Flags: needinfo?(acrichton)
If LTO is not enabled in Cargo.toml, then using "cargo rustc $ARGS -- -C lto" (instead of "cargo build $ARGS") will enable it, passing it only to the final rustc invocation.
@froydnj, out of curiosity, your mail indicates: > Compiling without LTO imposes significant binary size costs. I personally wouldn't expect *that* much of a cost here. If you've got some analysis available mind if I take a look at it? I'm curiuos if there's a rustc bug lying here in wait? Additionally, is there perhaps a better thread to discuss this on?
(In reply to Alex Crichton [:acrichto] from comment #15) > @froydnj, out of curiosity, your mail indicates: > > > Compiling without LTO imposes significant binary size costs. > > I personally wouldn't expect *that* much of a cost here. If you've got some > analysis available mind if I take a look at it? I'm curiuos if there's a > rustc bug lying here in wait? I've got some basic numbers close at hand; a nightly Firefox on my Linux machine: froydnj@hawkeye:~$ size ~/firefox/libxul.so text data bss dec hex filename 76369145 5008808 792833 82170786 4e5d3a2 /home/froydnj/firefox/libxul.so ...and the same changeset, compiled with identical options, but with Rust LTO disabled: froydnj@hawkeye:~/scratch$ size firefox/libxul.so text data bss dec hex filename 81458423 5172824 792899 87424146 535fc92 firefox/libxul.so so over a 5MB difference in text size, and a decent chunk of data. I don't know whether the numbers are radically different on Mac or Windows; those would be interesting to look at. Note that's *total* size for libxul, of which the Rust code is a relatively small part, so the benefits solely for Rust code are even larger. I haven't looked into this very carefully, but from what I remember, the problems are something like: * Non-LTO builds toss many `pub` symbols into the PLT; LTO does much less of this. * LTO can therefore do a better job of inlining. * LTO can get rid of parts of the rust stdlib. * LTO may enable the linker's dead code stripping to do a better job (e.g. `pub` symbols are candidates for dead code with LTO). I can investigate further! > Additionally, is there perhaps a better thread to discuss this on? There probably is, but I'm not sure what the right place is. Perhaps an internals.rust-lang.org thread?
Awesome, thanks! I've posted https://internals.rust-lang.org/t/rust-staticlibs-and-optimizing-for-size/5746 with some thoughts and what I think is an analysis of what's happening here. Probably best if we continue this discussion there.
(In reply to Matt Brubeck (:mbrubeck) from comment #18) > Created attachment 8895959 [details] This patch disables LTO by default, and uses `cargo rustc` to opt in to LTO for release builds in automation, as suggested in comment 14.
Should downstream add CARGO_RUSTCFLAGS_RELEASE="-C lto" to their mozconfigs for Firefox 57+? Or do you plan to implicitly enable it for --enable-release?
Flags: needinfo?(nfroyd)
Comment on attachment 8895959 [details] Bug 1386371 - Disable LTO by default, but enable in automation. https://reviewboard.mozilla.org/r/167224/#review172486 r- only because we should handle this differently, as Jan suggests, but this is great otherwise! ::: build/mozconfig.rust:9 (Diff revision 1) > +# Enable LTO in release builds. > +CARGO_RUSTCFLAGS_RELEASE="-C lto" This is an attractive idea, but I think we should tie this to `--enable-release` and `DEVELOPER_OPTIONS` in rules.mk ::: config/rules.mk:868 (Diff revision 1) > +ifndef MOZ_DEBUG_RUST > +cargo_rustc_flags += $(CARGO_RUSTCFLAGS_RELEASE) > +endif ...so here we'd be checking some setting of `DEVELOPER_OPTIONS`. It might even be possible that we only `--enable-release` for non-debug builds, so we'd only need to check one thing. ::: config/rules.mk:949 (Diff revision 1) > # > # but, given the idiosyncracies of make, can also be called without arguments: > # > # $(call CARGO_BUILD) > define CARGO_BUILD > -$(call RUN_CARGO,build,$(1)) > +$(call RUN_CARGO,rustc,$(1)) This is really rather clever; I didn't realize `cargo rustc` was powerful enough to build the entire dependency graph.
Attachment #8895959 - Flags: review?(nfroyd) → review-
(In reply to Jan Beich from comment #20) > Should downstream add CARGO_RUSTCFLAGS_RELEASE="-C lto" to their mozconfigs > for Firefox 57+? Or do you plan to implicitly enable it for --enable-release? It will be implicitly enabled via --enable-release; see comment 21.
Flags: needinfo?(nfroyd)
Updated the patch to enable LTO automatically for --enable-release builds. https://treeherder.mozilla.org/#/jobs?repo=try&revision=006c0fb406f2b9f81a95426bd2ab258817287d67
(In reply to Jan Beich from comment #20) > Should downstream add CARGO_RUSTCFLAGS_RELEASE="-C lto" to their mozconfigs > for Firefox 57+? Or do you plan to implicitly enable it for --enable-release? I'm a bit lost here - i've heard a lot about this --enable-release option recently, but i'm not sure when it should be used, and how 'different' it is from MOZILLA_OFFICIAL=1 in the env (something coming from the old days, maybe deprecated?) and/or --enable-official-branding (which is for the moz branding) - does the latter automagically set --enable-release ? Are distributors supposed to use --enable-release when packaging release/betas ?
(In reply to Landry Breuil (:gaston) from comment #27) > I'm a bit lost here - i've heard a lot about this --enable-release option > recently, but i'm not sure when it should be used, and how 'different' it is > from MOZILLA_OFFICIAL=1 in the env (something coming from the old days, > maybe deprecated?) and/or --enable-official-branding (which is for the moz > branding) - does the latter automagically set --enable-release ? Are > distributors supposed to use --enable-release when packaging release/betas ? From reading the code it seems MOZILLA_OFFICIAL implies --enable-release, which implies DEVELOPER_OPTIONS is unset.
(In reply to Jan Alexander Steffens from comment #28) > (In reply to Landry Breuil (:gaston) from comment #27) > > I'm a bit lost here - i've heard a lot about this --enable-release option > > recently, but i'm not sure when it should be used, and how 'different' it is > > from MOZILLA_OFFICIAL=1 in the env (something coming from the old days, > > maybe deprecated?) and/or --enable-official-branding (which is for the moz > > branding) - does the latter automagically set --enable-release ? Are > > distributors supposed to use --enable-release when packaging release/betas ? > > From reading the code it seems MOZILLA_OFFICIAL implies --enable-release, > which implies DEVELOPER_OPTIONS is unset. Do you mean https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/test/configure/test_toolkit_moz_configure.py#75 ? I dont understand enough of this voodoo to figure out if setting MOZILLA_OFFICIAL in the env *will* set --enable-release...
Comment on attachment 8895959 [details] Bug 1386371 - Disable LTO by default, but enable in automation. https://reviewboard.mozilla.org/r/167224/#review172728 This looks great, thanks. Two minor things below. ::: config/rules.mk:867 (Diff revision 4) > endif > endif > > +# These flags are passed via `cargo rustc` and only apply to the final rustc > +# invocation (i.e., only the top-level crate, not its dependencies). > +cargo_rustc_flags = $(CARGO_RUSTCFLAGS) Are we leaving `CARGO_RUSTCFLAGS` here for symmetry with `CARGOFLAGS`, above? ::: config/rules.mk:1030 (Diff revision 4) > endif # HOST_RUST_LIBRARY_FILE > > ifdef RUST_PROGRAMS > force-cargo-program-build: > $(REPORT_BUILD) > - $(call CARGO_BUILD,$(target_cargo_env_vars)) $(addprefix --bin ,$(RUST_CARGO_PROGRAMS)) $(cargo_target_flag) > + $(call CARGO_BUILD,$(target_cargo_env_vars)) $(addprefix --bin ,$(RUST_CARGO_PROGRAMS)) $(cargo_target_flag) -- $(cargo_rustc_flags) I see from the try run that this is passing `-C lto` for geckodriver...my sense is that we aren't super concerned about the runtime performance of geckodriver (normal release builds should be enough) and if we are super concerned about the runtime performance of geckodriver (or other `RUST_PROGRAMS`) and LTO would actually do something about that performance, those programs should be enabling LTO themselves. LTO on Rust programs isn't the huge space win that LTO is on staticlibs--and the space win is most of the reason we enable it for staticlibs--so I think we can drop this change.
Attachment #8895959 - Flags: review?(nfroyd) → review+
Comment on attachment 8895959 [details] Bug 1386371 - Disable LTO by default, but enable in automation. https://reviewboard.mozilla.org/r/167224/#review172728 > Are we leaving `CARGO_RUSTCFLAGS` here for symmetry with `CARGOFLAGS`, above? Yes, I think it will be useful for customizing local builds via mozconfig. For example, the people working on code size optimization might want to build with various combinations of LTO and other rustc flags. Now that we only pass this when building libraries, maybe it should have a more specific name. CARGO_LIB_RUSTCFLAGS?
Pushed by mbrubeck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d75f07915a48 Disable LTO by default, but enable in automation. r=froydnj
(In reply to Nathan Froyd [:froydnj] from comment #16) > * Non-LTO builds toss many `pub` symbols into the PLT; LTO does much less of > this. I'm a bit concerned of this. We probably should measure what this means for linker of libxul, especially the MSVC linker. It already takes me over 2mins to link when rust code changes.
https://hg.mozilla.org/mozilla-central/rev/d75f07915a48
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
What is the correct way to build with lto on a local build? There was talk of an --enable-rust-lto mozconfig option but I don't see it in the patch. Whatever the correct solution is, would be good to document somewhere.
Flags: needinfo?(nfroyd)
Added to the mozconfig instructions at https://public.etherpad-mozilla.org/p/stylo : To enable link-time optimization (LTO) [slows down builds, but reduces code size and may improve performance]: CARGO_RUSTCFLAGS=-Clto or "ac_add_options --enable-release" to the full set of optimizations used in release builds, including LTO.
Flags: needinfo?(nfroyd)
Too late for 56. Mass won't fix for 56.
status-firefox56: affected → wontfix
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: