Closed Bug 1411081 Opened 7 years ago Closed 7 years ago

Use opt-level=1 for Rust code by default

Categories

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

Product:
Firefox Build System
Component:
General
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

rust-only.svg
63.82 KB, image/svg+xml
Details
opt1.svg
47.24 KB, image/svg+xml
Details
Bug 1411081 - Foster debugging of old-configure.vars;
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1411081 - Move --enable-optimize/MOZ_OPTIMIZE to moz.configure;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1411081 - Derive Rust compiler flags in configure;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1411081 - Use opt-level=1 for rustc in default configuration;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1411081 - Move assignment of default value for MOZ_OPTIMIZE_FLAGS;
59 bytes, text/x-review-board-request
glandium
: review+
Details 
Manish was benchmarking builds on an i9-7960X (16 physical / 32 virtual CPU cores).

Looking at the build logs and resource usage during the build, Rust is a long pole by 5+ minutes at the end of builds when using the default build configuration. Adding --enable-debug (which has the side-effect of switching from the "release" to "dev" Cargo profile and from opt-level=2 to opt-level=1 makes the Rust parts of the build several *minutes* faster.

Given that Rust is a long pole by several minutes due to opt-level=2, I think we should consider changing the default to opt-level=1. If not globally, we should consider this for crates in the hot path or crates that >1 minute to build, like style and bindgen.
That's not the only difference. Another one being that opt builds have lto enabled.
Also, please define "by default" in the summary, because I doubt you intend released builds to use opt-level=1.
"by default" == fresh checkout without a mozconfig. i.e. "default developer build."

We'll want --enable-release (or similar) to increase optimizations to something more suitable for shipping.

Manish seemed to think that opt-level was responsible for most of the time difference. Although I'm not sure if he manually tweaked opt-level or edited emitter.py to also adjust LTO as well. (I /think/ it was just adding --enable-debug, which has the side-effect of opt-level=1, lto=false, and codegen-units=4.
What I did was --enable-debug, which made the rust build finish just after the (super parallel) C++ build, bringing compile times down to ~10m from ~15m. This also turns off LTO, yes.

I haven't yet been able to get codegen-units to properly utilize parallelism; need to look into this more.

I believe --enable-debug would be faster if it didn't compile bindgen in debug mode. I'm not 100% sure of this, since a lot of the time in bindgen is just spent in libclang and I don't know what the ratios are here. We'll need to push on https://github.com/rust-lang/cargo/issues/2424 to make this happen. (I should also add -Ctarget-cpu=native to cargo for build scripts)
> which made the rust build finish just after the

(Note that the Rust build doesn't start immediately, so this can actually vary. We may be able to get some wins if we prioritize doing Rust work on at least a single process first on any build that's -j4 or more)
LTO can be extra slow because of https://github.com/rust-lang/rust/issues/43212
There's also https://github.com/rust-lang/cargo/issues/4280 but it's hard to tell how much it affects us. geckoservo is the obvious case of having a crate-type of both staticlib and rlib, which actually doesn't matter much except for disk space, but 
there might be more subtle cases of things we're building that we end up not needing, and they're just hidden in plain sight because there are so many crates being built.

Relatedly, here's something I dug from an email thread I've been involved in a few months back (quoting myself replying to myself):

> > See the attached svg. This is from running cargo for gkrust with no C++
> > happening at the same time (with rust 1.20). This is invoking `make -j16
> > -C objdir/toolkit/library/rust target`. (note the svg only shows rustc
> > processes, but one can venture guesses as to what happens in some of the
> > blanks)
>
> > It seems like the core problem is that style simply can't start building
> > early because style needs its build script to have run, which requires
> > bindgen having been built, which requires syntex_syntax having been
> > built. syntex_syntax alone takes more than a minute and a half to build.
>
> Attached is another timeline when building with opt-level=1 and
> lto=false. I first tried with opt-level=0, but somehow, the build
> script for the style crate segfaults in that case. This does make
> the style crate build script run in 35s instead of < 10, but it allows
> the style crate starting to build 45s earlier...
>
> This gives a possible way out: everything that does not actually end up
> in the final library (build scripts and dependencies for the build
> scripts) could be built with no or less optimizations.
>
> But cargo currently doesn't allow different options for "host" and
> "target", does it?

I'll attach the two mentioned svg files.
Attached image rust-only.svg — 
First mentioned SVG.
Attached image opt1.svg — 
Second SVG mentioned.
(In reply to Manish Goregaokar [:manishearth] from comment #5)
> > which made the rust build finish just after the
> 
> (Note that the Rust build doesn't start immediately, so this can actually
> vary. We may be able to get some wins if we prioritize doing Rust work on at
> least a single process first on any build that's -j4 or more)

Bug 1370296 moved it as early as we can without too much hacks/risk.

Related: another quote from the same thread:

> > >   - Rust 1.19 is slower than 1.18. Possible issues with cargo job
> > > server? https://github.com/rust-lang/rust/issues/43211
> >
> > Michael or glandium, what do we need to do next to figure out this Rust 1.19
> > regression? There was a hypothesis that cargo and make jobs were thrashing?
>
> AFAICT there are two issues: one is that cargo doesn't seem to win the
> "race" with make frequently enough (but I'm not excluding that cargo is
> not trying to parallelize very much in the first place), and another is
> that cargo is non-deterministic in what it tries to build first, and it
> frequently happens that it ends up starting to build the largest crates
> later than necessary.
>
> Combine the latter with the fact that AIUI there's no trick you can do
> in a Cargo.toml to force some crates to be built earlier than others,
> and we got ourselves in a corner on the gecko side.
> This gives a possible way out: everything that does not actually end up
> in the final library (build scripts and dependencies for the build
> scripts) could be built with no or less optimizations.

This is interesting, I'd expect it to go the other way around, but it didn't occur to me that syntex_syntax is a bottleneck. We use that for codegen but I don't think its performance should be a bottleneck so compiling it in debug mode should help.

(OTOH bindgen itself may benefit from being compiled in release mode; it does a lot of work)
On my i7-6700K with a default build config and rust 1.21, the style crate takes ~4 minutes and geckoservo takes ~2.5 minutes. These two are in a long pole. Everything else takes ~3 minutes. Here is a partial log from `mach build toolkit/library/rust`:

 0:00.25 /usr/bin/make -C /home/gps/src/firefox/objdir -j8 -s backend
 0:00.82 /usr/bin/make -C toolkit/library/rust -j8 -s
 0:00.84 force-cargo-library-build
 0:02.04    Compiling glob v0.2.11
 0:02.04    Compiling log v0.3.8
 0:02.04    Compiling libloading v0.4.0
 0:02.04    Compiling nom v1.2.4
 0:02.04    Compiling libc v0.2.24
 0:02.04    Compiling regex-syntax v0.4.1
 0:02.04    Compiling toml v0.2.1
 0:02.04    Compiling rayon-core v1.2.0
 0:02.54    Compiling strsim v0.6.0
 0:02.90    Compiling unicode-width v0.1.4
 0:03.00    Compiling unicode-segmentation v1.1.0
 0:03.11    Compiling siphasher v0.2.1
 0:03.23    Compiling ident_case v1.0.0
 0:03.66    Compiling bitflags v0.7.0
 0:03.67    Compiling ansi_term v0.9.0
 0:03.93    Compiling webrender v0.52.1 (file:///home/gps/src/firefox/gfx/webrender)
 0:03.94    Compiling quote v0.3.15
 0:04.07    Compiling void v1.0.2
 0:04.34    Compiling lazy_static v0.2.8
 0:04.40    Compiling same-file v0.1.3
 0:04.59    Compiling bitflags v0.8.2
 0:04.74    Compiling heapsize v0.4.0
 0:04.84    Compiling cfg-if v0.1.1
 0:04.84    Compiling khronos_api v1.0.1
 0:04.96    Compiling procedural-masquerade v0.1.1
 0:05.13    Compiling bitflags v0.9.1
 0:05.27    Compiling term v0.4.5
 0:05.42    Compiling rustc-serialize v0.3.24
 0:05.44    Compiling peeking_take_while v0.1.2
 0:05.47    Compiling vec_map v0.8.0
 0:05.60    Compiling unicode-xid v0.0.4
 0:05.76    Compiling utf8-ranges v1.0.0
 0:05.86    Compiling pkg-config v0.3.9
 0:05.93    Compiling phf_shared v0.7.21
 0:06.03    Compiling unreachable v0.1.1
 0:06.29    Compiling itertools v0.5.10
 0:06.41    Compiling coco v0.1.1
 0:06.55    Compiling term_size v0.3.0
 0:06.96    Compiling rand v0.3.15
 0:07.56    Compiling thread-id v3.1.0
 0:07.61    Compiling atty v0.2.2
 0:07.96    Compiling memchr v1.0.1
 0:07.96    Compiling unicode-bidi v0.3.3
 0:08.53    Compiling xml-rs v0.3.6
 0:08.54    Compiling nsstring_vendor v0.1.0 (file:///home/gps/src/firefox/servo/components/style/gecko_bindings/nsstring_vendor)
 0:08.58    Compiling nsstring v0.1.0 (file:///home/gps/src/firefox/xpcom/rust/nsstring)
 0:09.83    Compiling cexpr v0.2.0
 0:10.50    Compiling synom v0.11.2
 0:10.92    Compiling encoding_rs v0.7.1
 0:10.97    Compiling clang-sys v0.21.0
 0:11.31    Compiling owning_ref v0.3.3
 0:11.85    Compiling walkdir v1.0.7
 0:11.93    Compiling cubeb-ffi v0.0.1 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/cubeb-ffi)
 0:12.59    Compiling mp4parse v0.8.0 (file:///home/gps/src/firefox/media/libstagefright/binding/mp4parse)
 0:12.81    Compiling num_cpus v1.6.0
 0:13.40    Compiling net2 v0.2.31
 0:14.71    Compiling time v0.1.36
 0:14.96    Compiling hashglobe v0.1.0 (file:///home/gps/src/firefox/servo/components/hashglobe)
 0:15.13    Compiling freetype v0.3.0
 0:15.35    Compiling pulse-ffi v0.1.0 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/pulse-ffi)
 0:16.25    Compiling libudev-sys v0.1.3 (file:///home/gps/src/firefox/dom/webauthn/libudev-sys)
 0:16.41    Compiling iovec v0.1.0
 0:16.73    Compiling memmap v0.5.2
 0:16.85    Compiling arrayvec v0.3.23
 0:17.47    Compiling servo_arc v0.0.1 (file:///home/gps/src/firefox/servo/components/servo_arc)
 0:17.57    Compiling ordered-float v0.4.0
 0:17.82    Compiling phf v0.7.21
 0:18.07    Compiling textwrap v0.6.0
 0:18.27    Compiling libcubeb-sys v0.1.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-api/libcubeb-sys)
 0:18.34    Compiling cubeb-backend v0.2.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-backend)
 0:18.48    Compiling aho-corasick v0.6.3
 0:18.49    Compiling phf_generator v0.7.21
 0:18.66    Compiling nserror v0.1.0 (file:///home/gps/src/firefox/xpcom/rust/nserror)
 0:18.66    Compiling thread_local v0.3.3
 0:18.85    Compiling syn v0.11.11
 0:18.93    Compiling idna v0.1.4
 0:19.17    Compiling syntex_pos v0.58.1
 0:19.40    Compiling gl_generator v0.5.3
 0:19.70    Compiling mp4parse_capi v0.8.0 (file:///home/gps/src/firefox/media/libstagefright/binding/mp4parse_capi)
 0:19.93    Compiling fallible v0.0.1 (file:///home/gps/src/firefox/servo/components/fallible)
 0:20.15    Compiling pulse v0.1.0 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/pulse-rs)
 0:20.33    Compiling bytes v0.4.5
 0:20.83    Compiling mio v0.6.9
 0:22.22    Compiling lru_cache v0.0.1 (file:///home/gps/src/firefox/servo/components/lru_cache)
 0:22.65    Compiling parking_lot_core v0.2.4
 0:24.58    Compiling libudev v0.2.0
 0:25.39    Compiling cubeb v0.3.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-api)
 0:25.57    Compiling phf_codegen v0.7.21
 0:26.03    Compiling regex v0.2.2
 0:26.21    Compiling clap v2.25.0
 0:26.85    Compiling syntex_errors v0.58.1
 0:36.79    Compiling cubeb-pulse v0.0.1 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs)
 0:39.31    Compiling url v1.5.1
 0:42.01    Compiling parking_lot v0.4.4
 0:43.45    Compiling gleam v0.4.8
 0:47.25    Compiling mio-uds v0.6.4
 0:47.61    Compiling selectors v0.19.0 (file:///home/gps/src/firefox/servo/components/selectors)
 0:48.26    Compiling rayon v0.8.2
 0:50.28    Compiling encoding_c v0.8.0
 0:50.87    Compiling encoding_glue v0.1.0 (file:///home/gps/src/firefox/intl/encoding_glue)
 0:50.92    Compiling syntex_syntax v0.58.1
 0:51.46    Compiling rust_url_capi v0.0.1 (file:///home/gps/src/firefox/netwerk/base/rust-url-capi)
 0:51.88    Compiling env_logger v0.4.3
 0:52.91    Compiling cssparser v0.22.0
 0:53.29    Compiling synstructure v0.5.2
 0:53.74    Compiling cssparser-macros v0.3.0
 0:53.95    Compiling serde_derive_internals v0.15.1
 0:54.36    Compiling darling_core v0.2.0
 0:55.32    Compiling malloc_size_of_derive v0.0.1 (file:///home/gps/src/firefox/servo/components/malloc_size_of_derive)
 0:59.29    Compiling u2fhid v0.1.0 (file:///home/gps/src/firefox/dom/webauthn/u2f-hid-rs)
 1:07.77    Compiling serde_derive v1.0.8
 1:09.65    Compiling darling_macro v0.2.0
 1:12.26    Compiling darling v0.2.0
 1:12.58    Compiling style_derive v0.0.1 (file:///home/gps/src/firefox/servo/components/style_derive)
 1:29.80    Compiling serde v1.0.8
 1:38.67    Compiling bincode v0.9.1
 1:38.67    Compiling app_units v0.5.6
 1:38.67    Compiling bincode v0.8.0
 1:38.67    Compiling euclid v0.15.2
 1:39.68    Compiling audioipc v0.1.0 (file:///home/gps/src/firefox/media/audioipc/audioipc)
 1:46.70    Compiling style_traits v0.0.1 (file:///home/gps/src/firefox/servo/components/style_traits)
 2:27.65    Compiling syntex v0.58.1
 2:27.65    Compiling quasi v0.32.0
 2:27.65    Compiling aster v0.41.0
 2:31.01    Compiling quasi_codegen v0.32.0
 2:39.69    Compiling bindgen v0.29.1
 2:58.34    Compiling webrender_bindings v0.1.0 (file:///home/gps/src/firefox/gfx/webrender_bindings)
 3:21.56    Compiling style v0.0.1 (file:///home/gps/src/firefox/servo/components/style)
 7:48.94    Compiling geckoservo v0.0.1 (file:///home/gps/src/firefox/servo/ports/geckolib)
10:19.18    Compiling gkrust-shared v0.1.0 (file:///home/gps/src/firefox/toolkit/library/rust/shared)
10:20.41    Compiling gkrust v0.1.0 (file:///home/gps/src/firefox/toolkit/library/rust)
10:22.15     Finished release [optimized] target(s) in 620.7 secs
10:23.38 force-cargo-library-build
10:24.39     Finished release [optimized] target(s) in 0.0 secs

real    10m24.961s
user    21m4.445s
sys     0m17.307s


If I hack up the release profile to disable lto via the following patch:

diff --git a/python/mozbuild/mozbuild/frontend/emitter.py b/python/mozbuild/mozbuild/frontend/emitter.py
--- a/python/mozbuild/mozbuild/frontend/emitter.py
+++ b/python/mozbuild/mozbuild/frontend/emitter.py
@@ -494,6 +494,7 @@ class TreeMetadataEmitter(LoggingMixin):
                     expected_profile = {
                         'opt-level': 2,
                         'rpath': False,
+                        'lto': False,
                         'debug-assertions': False,
                         'panic': 'abort',
                     }
diff --git a/toolkit/library/gtest/rust/Cargo.toml b/toolkit/library/gtest/rust/Cargo.toml
--- a/toolkit/library/gtest/rust/Cargo.toml
+++ b/toolkit/library/gtest/rust/Cargo.toml
@@ -44,6 +44,7 @@ opt-level = 2
 rpath = false
 debug-assertions = false
 panic = "abort"
+lto = false

 [replace]
 "libudev-sys:0.1.3" = { path = "../../../../dom/webauthn/libudev-sys" }
diff --git a/toolkit/library/rust/Cargo.toml b/toolkit/library/rust/Cargo.toml
--- a/toolkit/library/rust/Cargo.toml
+++ b/toolkit/library/rust/Cargo.toml
@@ -42,6 +42,7 @@ panic = "abort"

 [profile.release]
 opt-level = 2
+lto = false
 rpath = false
 debug-assertions = false
 panic = "abort"


... we get:

$ time ./mach build toolkit/library/rust
 0:00.22 /usr/bin/make -C /home/gps/src/firefox/objdir -j8 -s backend
 0:00.34 /usr/bin/make -C toolkit/library/rust -j8 -s
 0:00.36 force-cargo-library-build
 0:01.50    Compiling utf8-ranges v1.0.0
 0:01.50    Compiling vec_map v0.8.0
 0:01.50    Compiling libloading v0.4.0
 0:01.50    Compiling bitflags v0.8.2
 0:01.50    Compiling rayon-core v1.2.0
 0:01.50    Compiling quote v0.3.15
 0:01.51    Compiling heapsize v0.4.0
 0:01.51    Compiling unicode-segmentation v1.1.0
 0:01.78    Compiling bitreader v0.3.0
 0:02.01    Compiling lazy_static v0.2.8
 0:02.07    Compiling unicode-width v0.1.4
 0:02.31    Compiling odds v0.2.25
 0:02.41    Compiling khronos_api v1.0.1
 0:02.42    Compiling bitflags v0.7.0
 0:02.42    Compiling procedural-masquerade v0.1.1
 0:02.44    Compiling byteorder v1.0.0
 0:02.56    Compiling thread_profiler v0.1.1
 0:02.71    Compiling boxfnonce v0.0.3
 0:02.80    Compiling bitflags v0.9.1
 0:02.88    Compiling peeking_take_while v0.1.2
 0:02.92    Compiling term v0.4.5
 0:02.96    Compiling libc v0.2.24
 0:03.10    Compiling ident_case v1.0.0
 0:03.14    Compiling unicode-xid v0.0.4
 0:03.21    Compiling fnv v1.0.5
 0:03.23    Compiling toml v0.2.1
 0:03.27    Compiling cfg-if v0.1.1
 0:03.48    Compiling matches v0.1.4
 0:03.48    Compiling regex-syntax v0.4.1
 0:03.60    Compiling rustc-serialize v0.3.24
 0:03.60    Compiling pkg-config v0.3.9
 0:03.83    Compiling webrender v0.52.1 (file:///home/gps/src/firefox/gfx/webrender)
 0:03.93    Compiling void v1.0.2
 0:04.04    Compiling ansi_term v0.9.0
 0:04.20    Compiling nom v1.2.4
 0:05.02    Compiling smallvec v0.4.3
 0:05.53    Compiling lazycell v0.4.0
 0:05.80    Compiling strsim v0.6.0
 0:06.23    Compiling siphasher v0.2.1
 0:06.92    Compiling slab v0.3.0
 0:07.45    Compiling log v0.3.8
 0:07.45    Compiling same-file v0.1.3
 0:07.90    Compiling glob v0.2.11
 0:08.14    Compiling cubeb-core v0.1.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-core)
 0:08.42    Compiling itertools v0.5.10
 0:08.91    Compiling encoding_rs v0.7.1
 0:09.32    Compiling phf_shared v0.7.21
 0:09.42    Compiling synom v0.11.2
 0:09.77    Compiling owning_ref v0.3.3
 0:10.27    Compiling xml-rs v0.3.6
 0:10.50    Compiling nsstring v0.1.0 (file:///home/gps/src/firefox/xpcom/rust/nsstring)
 0:10.58    Compiling nsstring_vendor v0.1.0 (file:///home/gps/src/firefox/servo/components/style/gecko_bindings/nsstring_vendor)
 0:10.82    Compiling semver v0.6.0
 0:12.62    Compiling fxhash v0.2.1
 0:12.88    Compiling cubeb-ffi v0.0.1 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/cubeb-ffi)
 0:13.16    Compiling num-integer v0.1.33
 0:13.70    Compiling mp4parse v0.8.0 (file:///home/gps/src/firefox/media/libstagefright/binding/mp4parse)
 0:13.73    Compiling unicode-bidi v0.3.3
 0:13.79    Compiling nodrop v0.1.9
 0:13.81    Compiling iovec v0.1.0
 0:14.17    Compiling memchr v1.0.1
 0:14.23    Compiling rand v0.3.15
 0:14.39    Compiling freetype v0.3.0
 0:14.84    Compiling thread-id v3.1.0
 0:15.19    Compiling time v0.1.36
 0:16.19    Compiling net2 v0.2.31
 0:17.09    Compiling memmap v0.5.2
 0:17.61    Compiling num_cpus v1.6.0
 0:17.62    Compiling hashglobe v0.1.0 (file:///home/gps/src/firefox/servo/components/hashglobe)
 0:17.66    Compiling pulse-ffi v0.1.0 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/pulse-ffi)
 0:17.91    Compiling unreachable v0.1.1
 0:17.94    Compiling libudev-sys v0.1.3 (file:///home/gps/src/firefox/dom/webauthn/libudev-sys)
 0:17.98    Compiling coco v0.1.1
 0:18.28    Compiling cexpr v0.2.0
 0:18.98    Compiling term_size v0.3.0
 0:19.46    Compiling atty v0.2.2
 0:19.58    Compiling dtoa-short v0.3.1
 0:19.71    Compiling walkdir v1.0.7
 0:19.81    Compiling libcubeb-sys v0.1.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-api/libcubeb-sys)
 0:19.89    Compiling cubeb-backend v0.2.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-backend)
 0:20.17    Compiling phf v0.7.21
 0:20.31    Compiling syn v0.11.11
 0:20.79    Compiling clang-sys v0.21.0
 0:21.03    Compiling nserror v0.1.0 (file:///home/gps/src/firefox/xpcom/rust/nserror)
 0:21.48    Compiling syntex_pos v0.58.1
 0:21.87    Compiling arrayvec v0.3.23
 0:22.04    Compiling servo_arc v0.0.1 (file:///home/gps/src/firefox/servo/components/servo_arc)
 0:22.32    Compiling bytes v0.4.5
 0:22.68    Compiling aho-corasick v0.6.3
 0:22.78    Compiling gl_generator v0.5.3
 0:23.22    Compiling mp4parse_capi v0.8.0 (file:///home/gps/src/firefox/media/libstagefright/binding/mp4parse_capi)
 0:23.76    Compiling parking_lot_core v0.2.4
 0:24.25    Compiling idna v0.1.4
 0:24.78    Compiling mio v0.6.9
 0:25.66    Compiling ordered-float v0.4.0
 0:26.31    Compiling thread_local v0.3.3
 0:26.87    Compiling fallible v0.0.1 (file:///home/gps/src/firefox/servo/components/fallible)
 0:27.29    Compiling pulse v0.1.0 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/pulse-rs)
 0:27.83    Compiling textwrap v0.6.0
 0:29.21    Compiling cubeb v0.3.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-api)
 0:29.39    Compiling libudev v0.2.0
 0:29.44    Compiling phf_generator v0.7.21
 0:29.71    Compiling lru_cache v0.0.1 (file:///home/gps/src/firefox/servo/components/lru_cache)
 0:29.85    Compiling syntex_errors v0.58.1
 0:30.06    Compiling encoding_c v0.8.0
 0:30.13    Compiling encoding_glue v0.1.0 (file:///home/gps/src/firefox/intl/encoding_glue)
 0:30.43    Compiling parking_lot v0.4.4
 0:30.68    Compiling regex v0.2.2
 0:31.19    Compiling url v1.5.1
 0:31.73    Compiling gleam v0.4.8
 0:31.75    Compiling cubeb-pulse v0.0.1 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs)
 0:34.33    Compiling mio-uds v0.6.4
 0:35.28    Compiling clap v2.25.0
 0:44.06    Compiling phf_codegen v0.7.21
 0:44.06    Compiling rayon v0.8.2
 0:44.56    Compiling syntex_syntax v0.58.1
 0:47.24    Compiling rust_url_capi v0.0.1 (file:///home/gps/src/firefox/netwerk/base/rust-url-capi)
 0:49.16    Compiling selectors v0.19.0 (file:///home/gps/src/firefox/servo/components/selectors)
 0:49.75    Compiling serde_derive_internals v0.15.1
 0:52.63    Compiling synstructure v0.5.2
 0:53.23    Compiling cssparser v0.22.0
 0:53.76    Compiling darling_core v0.2.0
 0:55.58    Compiling cssparser-macros v0.3.0
 0:59.32    Compiling malloc_size_of_derive v0.0.1 (file:///home/gps/src/firefox/servo/components/malloc_size_of_derive)
 1:02.35    Compiling env_logger v0.4.3
 1:04.85    Compiling serde_derive v1.0.8
 1:05.38    Compiling u2fhid v0.1.0 (file:///home/gps/src/firefox/dom/webauthn/u2f-hid-rs)
 1:10.07    Compiling darling_macro v0.2.0
 1:14.32    Compiling darling v0.2.0
 1:14.85    Compiling style_derive v0.0.1 (file:///home/gps/src/firefox/servo/components/style_derive)
 1:31.86    Compiling serde v1.0.8
 1:40.10    Compiling bincode v0.8.0
 1:40.10    Compiling app_units v0.5.6
 1:40.10    Compiling euclid v0.15.2
 1:40.10    Compiling bincode v0.9.1
 1:41.01    Compiling audioipc v0.1.0 (file:///home/gps/src/firefox/media/audioipc/audioipc)
 1:41.55    Compiling webrender_api v0.52.1 (file:///home/gps/src/firefox/gfx/webrender_api)
 1:41.55    Compiling malloc_size_of v0.0.1 (file:///home/gps/src/firefox/servo/components/malloc_size_of)
 1:41.55    Compiling plane-split v0.6.0
 1:43.02    Compiling audioipc-server v0.1.0 (file:///home/gps/src/firefox/media/audioipc/server)
 1:43.02    Compiling audioipc-client v0.1.0 (file:///home/gps/src/firefox/media/audioipc/client)
 1:47.04    Compiling style_traits v0.0.1 (file:///home/gps/src/firefox/servo/components/style_traits)
 2:23.32    Compiling aster v0.41.0
 2:23.32    Compiling syntex v0.58.1
 2:23.32    Compiling quasi v0.32.0
 2:26.45    Compiling quasi_codegen v0.32.0
 2:33.96    Compiling bindgen v0.29.1
 2:53.64    Compiling webrender_bindings v0.1.0 (file:///home/gps/src/firefox/gfx/webrender_bindings)
 3:14.84    Compiling style v0.0.1 (file:///home/gps/src/firefox/servo/components/style)
 7:37.44    Compiling geckoservo v0.0.1 (file:///home/gps/src/firefox/servo/ports/geckolib)
10:05.10    Compiling gkrust-shared v0.1.0 (file:///home/gps/src/firefox/toolkit/library/rust/shared)
10:06.15    Compiling gkrust v0.1.0 (file:///home/gps/src/firefox/toolkit/library/rust)
10:08.13     Finished release [optimized] target(s) in 606.63 secs
10:09.27 force-cargo-library-build
10:10.33     Finished release [optimized] target(s) in 0.0 secs

real    10m10.913s
user    21m22.427s
sys     0m18.388s


So disabling lto with opt-level=2 didn't appear to change much.

If we switch to opt-level=1 with the following patch:

diff --git a/python/mozbuild/mozbuild/frontend/emitter.py b/python/mozbuild/mozbuild/frontend/emitter.py
--- a/python/mozbuild/mozbuild/frontend/emitter.py
+++ b/python/mozbuild/mozbuild/frontend/emitter.py
@@ -492,7 +492,7 @@ class TreeMetadataEmitter(LoggingMixin):
                     }
                 else:
                     expected_profile = {
-                        'opt-level': 2,
+                        'opt-level': 1,
                         'rpath': False,
                         'debug-assertions': False,
                         'panic': 'abort',
diff --git a/toolkit/library/gtest/rust/Cargo.toml b/toolkit/library/gtest/rust/Cargo.toml
--- a/toolkit/library/gtest/rust/Cargo.toml
+++ b/toolkit/library/gtest/rust/Cargo.toml
@@ -40,7 +40,7 @@ codegen-units = 4
 panic = "abort"

 [profile.release]
-opt-level = 2
+opt-level = 1
 rpath = false
 debug-assertions = false
 panic = "abort"
diff --git a/toolkit/library/rust/Cargo.toml b/toolkit/library/rust/Cargo.toml
--- a/toolkit/library/rust/Cargo.toml
+++ b/toolkit/library/rust/Cargo.toml
@@ -41,7 +41,7 @@ codegen-units = 4
 panic = "abort"

 [profile.release]
-opt-level = 2
+opt-level = 1
 rpath = false
 debug-assertions = false
 panic = "abort"

... we get:

)$ time ./mach build toolkit/library/rust
 0:00.14 /usr/bin/make -C /home/gps/src/firefox/objdir -j8 -s backend
 0:00.21 /usr/bin/make -C toolkit/library/rust -j8 -s
 0:00.24 force-cargo-library-build
 0:01.26    Compiling regex-syntax v0.4.1
 0:01.26    Compiling stable_deref_trait v1.0.0
 0:01.26    Compiling siphasher v0.2.1
 0:01.26    Compiling void v1.0.2
 0:01.26    Compiling same-file v0.1.3
 0:01.26    Compiling term v0.4.5
 0:01.26    Compiling nom v1.2.4
 0:01.26    Compiling utf8-ranges v1.0.0
 0:01.62    Compiling procedural-masquerade v0.1.1
 0:01.82    Compiling ansi_term v0.9.0
 0:01.92    Compiling dtoa v0.4.2
 0:02.09    Compiling libloading v0.4.0
 0:02.23    Compiling precomputed-hash v0.1.1
 0:02.39    Compiling peeking_take_while v0.1.2
 0:02.54    Compiling lazy_static v0.2.8
 0:02.56    Compiling khronos_api v1.0.1
 0:02.68    Compiling cfg-if v0.1.1
 0:02.92    Compiling unicode-width v0.1.4
 0:02.94    Compiling itoa v0.3.1
 0:03.02    Compiling unicode-xid v0.0.4
 0:03.06    Compiling fnv v1.0.5
 0:03.27    Compiling log v0.3.8
 0:03.38    Compiling strsim v0.6.0
 0:03.38    Compiling unicode-segmentation v1.1.0
 0:03.48    Compiling pkg-config v0.3.9
 0:03.68    Compiling quote v0.3.15
 0:03.69    Compiling smallvec v0.4.3
 0:04.03    Compiling matches v0.1.4
 0:04.36    Compiling rayon-core v1.2.0
 0:04.36    Compiling webrender v0.52.1 (file:///home/gps/src/firefox/gfx/webrender)
 0:04.47    Compiling bitflags v0.9.1
 0:04.76    Compiling error-chain v0.10.0
 0:04.84    Compiling ident_case v1.0.0
 0:05.06    Compiling glob v0.2.11
 0:05.23    Compiling bitflags v0.7.0
 0:05.29    Compiling rustc-serialize v0.3.24
 0:05.32    Compiling heapsize v0.4.0
 0:05.37    Compiling boxfnonce v0.0.3
 0:05.54    Compiling bitflags v0.8.2
 0:05.77    Compiling lazycell v0.4.0
 0:05.78    Compiling thread_profiler v0.1.1
 0:05.86    Compiling num-traits v0.1.39
 0:06.15    Compiling percent-encoding v1.0.0
 0:06.16    Compiling unicode-normalization v0.1.5
 0:06.30    Compiling smallbitvec v1.0.6
 0:07.41    Compiling toml v0.2.1
 0:07.43    Compiling libc v0.2.24
 0:07.61    Compiling binary-space-partition v0.1.2
 0:07.86    Compiling semver-parser v0.7.0
 0:07.99    Compiling slab v0.3.0
 0:08.45    Compiling vec_map v0.8.0
 0:08.97    Compiling odds v0.2.25
 0:09.35    Compiling unreachable v0.1.1
 0:09.62    Compiling owning_ref v0.3.3
 0:09.81    Compiling walkdir v1.0.7
 0:10.10    Compiling phf_shared v0.7.21
 0:10.28    Compiling itertools v0.5.10
 0:10.34    Compiling dtoa-short v0.3.1
 0:10.56    Compiling coco v0.1.1
 0:11.13    Compiling cexpr v0.2.0
 0:11.41    Compiling synom v0.11.2
 0:11.63    Compiling encoding_rs v0.7.1
 0:11.65    Compiling unicode-bidi v0.3.3
 0:12.10    Compiling cubeb-core v0.1.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-core)
 0:12.90    Compiling xml-rs v0.3.6
 0:13.40    Compiling cubeb-ffi v0.0.1 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/cubeb-ffi)
 0:13.46    Compiling clang-sys v0.21.0
 0:13.73    Compiling fxhash v0.2.1
 0:14.19    Compiling num-integer v0.1.33
 0:14.22    Compiling mp4parse v0.8.0 (file:///home/gps/src/firefox/media/libstagefright/binding/mp4parse)
 0:14.32    Compiling nsstring_vendor v0.1.0 (file:///home/gps/src/firefox/servo/components/style/gecko_bindings/nsstring_vendor)
 0:14.66    Compiling nsstring v0.1.0 (file:///home/gps/src/firefox/xpcom/rust/nsstring)
 0:14.76    Compiling term_size v0.3.0
 0:14.98    Compiling rand v0.3.15
 0:15.12    Compiling thread-id v3.1.0
 0:15.44    Compiling atty v0.2.2
 0:15.84    Compiling memchr v1.0.1
 0:15.94    Compiling semver v0.6.0
 0:16.39    Compiling freetype v0.3.0
 0:17.43    Compiling libudev-sys v0.1.3 (file:///home/gps/src/firefox/dom/webauthn/libudev-sys)
 0:17.44    Compiling pulse-ffi v0.1.0 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/pulse-ffi)
 0:17.48    Compiling memmap v0.5.2
 0:17.73    Compiling num_cpus v1.6.0
 0:17.88    Compiling hashglobe v0.1.0 (file:///home/gps/src/firefox/servo/components/hashglobe)
 0:18.31    Compiling time v0.1.36
 0:18.57    Compiling net2 v0.2.31
 0:18.96    Compiling iovec v0.1.0
 0:19.36    Compiling nodrop v0.1.9
 0:19.38    Compiling phf v0.7.21
 0:19.40    Compiling ordered-float v0.4.0
 0:19.75    Compiling syn v0.11.11
 0:20.05    Compiling cubeb-backend v0.2.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-backend)
 0:20.06    Compiling libcubeb-sys v0.1.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-api/libcubeb-sys)
 0:20.40    Compiling syntex_pos v0.58.1
 0:20.44    Compiling idna v0.1.4
 0:20.50    Compiling textwrap v0.6.0
 0:20.64    Compiling thread_local v0.3.3
 0:20.64    Compiling aho-corasick v0.6.3
 0:21.13    Compiling nserror v0.1.0 (file:///home/gps/src/firefox/xpcom/rust/nserror)
 0:21.56    Compiling phf_generator v0.7.21
 0:21.58    Compiling mp4parse_capi v0.8.0 (file:///home/gps/src/firefox/media/libstagefright/binding/mp4parse_capi)
 0:21.86    Compiling gl_generator v0.5.3
 0:22.02    Compiling pulse v0.1.0 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs/pulse-rs)
 0:22.47    Compiling fallible v0.0.1 (file:///home/gps/src/firefox/servo/components/fallible)
 0:22.69    Compiling libudev v0.2.0
 0:22.85    Compiling bytes v0.4.5
 0:23.68    Compiling servo_arc v0.0.1 (file:///home/gps/src/firefox/servo/components/servo_arc)
 0:23.68    Compiling arrayvec v0.3.23
 0:23.79    Compiling mio v0.6.9
 0:24.28    Compiling parking_lot_core v0.2.4
 0:24.60    Compiling cubeb v0.3.0 (file:///home/gps/src/firefox/media/cubeb-rs/cubeb-api)
 0:26.17    Compiling regex v0.2.2
 0:26.57    Compiling encoding_c v0.8.0
 0:27.17    Compiling encoding_glue v0.1.0 (file:///home/gps/src/firefox/intl/encoding_glue)
 0:27.32    Compiling clap v2.25.0
 0:28.00    Compiling phf_codegen v0.7.21
 0:28.23    Compiling syntex_errors v0.58.1
 0:29.33    Compiling cubeb-pulse v0.0.1 (file:///home/gps/src/firefox/media/libcubeb/cubeb-pulse-rs)
 0:32.00    Compiling url v1.5.1
 0:34.94    Compiling lru_cache v0.0.1 (file:///home/gps/src/firefox/servo/components/lru_cache)
 0:35.36    Compiling parking_lot v0.4.4
 0:36.67    Compiling gleam v0.4.8
 0:36.91    Compiling rayon v0.8.2
 0:37.44    Compiling mio-uds v0.6.4
 0:37.88    Compiling selectors v0.19.0 (file:///home/gps/src/firefox/servo/components/selectors)
 0:38.44    Compiling syntex_syntax v0.58.1
 0:40.14    Compiling cssparser v0.22.0
 0:40.15    Compiling serde_derive_internals v0.15.1
 0:41.54    Compiling darling_core v0.2.0
 0:41.91    Compiling cssparser-macros v0.3.0
 0:42.30    Compiling synstructure v0.5.2
 0:42.94    Compiling rust_url_capi v0.0.1 (file:///home/gps/src/firefox/netwerk/base/rust-url-capi)
 0:44.19    Compiling env_logger v0.4.3
 0:46.33    Compiling malloc_size_of_derive v0.0.1 (file:///home/gps/src/firefox/servo/components/malloc_size_of_derive)
 0:48.56    Compiling serde_derive v1.0.8
 0:50.57    Compiling u2fhid v0.1.0 (file:///home/gps/src/firefox/dom/webauthn/u2f-hid-rs)
 0:51.52    Compiling darling_macro v0.2.0
 0:53.99    Compiling darling v0.2.0
 0:54.37    Compiling style_derive v0.0.1 (file:///home/gps/src/firefox/servo/components/style_derive)
 0:59.93    Compiling serde v1.0.8
 1:09.23    Compiling euclid v0.15.2
 1:09.23    Compiling app_units v0.5.6
 1:09.23    Compiling bincode v0.8.0
 1:09.23    Compiling bincode v0.9.1
 1:10.19    Compiling audioipc v0.1.0 (file:///home/gps/src/firefox/media/audioipc/audioipc)
 1:10.54    Compiling webrender_api v0.52.1 (file:///home/gps/src/firefox/gfx/webrender_api)
 1:10.54    Compiling malloc_size_of v0.0.1 (file:///home/gps/src/firefox/servo/components/malloc_size_of)
 1:10.54    Compiling plane-split v0.6.0
 1:12.12    Compiling audioipc-server v0.1.0 (file:///home/gps/src/firefox/media/audioipc/server)
 1:12.12    Compiling audioipc-client v0.1.0 (file:///home/gps/src/firefox/media/audioipc/client)
 1:15.55    Compiling style_traits v0.0.1 (file:///home/gps/src/firefox/servo/components/style_traits)
 1:31.37    Compiling aster v0.41.0
 1:31.37    Compiling syntex v0.58.1
 1:31.37    Compiling quasi v0.32.0
 1:33.61    Compiling quasi_codegen v0.32.0
 1:38.29    Compiling bindgen v0.29.1
 1:59.21    Compiling webrender_bindings v0.1.0 (file:///home/gps/src/firefox/gfx/webrender_bindings)
 2:03.04    Compiling style v0.0.1 (file:///home/gps/src/firefox/servo/components/style)
 4:28.62    Compiling geckoservo v0.0.1 (file:///home/gps/src/firefox/servo/ports/geckolib)
 5:19.75    Compiling gkrust-shared v0.1.0 (file:///home/gps/src/firefox/toolkit/library/rust/shared)
 5:20.67    Compiling gkrust v0.1.0 (file:///home/gps/src/firefox/toolkit/library/rust)
 5:22.38     Finished release [optimized] target(s) in 321.11 secs
 5:23.26 force-cargo-library-build
 5:24.25     Finished release [optimized] target(s) in 0.0 secs

real    5m24.599s
user    13m5.630s
sys     0m16.662s


So ~123s instead of ~195s to get to the style crate. ~270s instead of ~470s to get to geckoservo. And ~325s vs ~625s overall.

The bulk of the slowness sure seems to be isolated to opt-level=2 vs 1.
(In reply to Mike Hommey [:glandium] from comment #1)
> That's not the only difference. Another one being that opt builds have lto
> enabled.

We disabled LTO by default in bug 1386371. It gets turned on by --enable-release.
Assignee: nobody → gps
Status: NEW → ASSIGNED
PSA: I'm not an expert on the Rust integration in the build system. And I've been away from writing moz.configure patches for over a month. So please give extra scrutiny during code review.
Comment on attachment 8921682 [details]
Bug 1411081 - Make ./configure --help work again;

https://reviewboard.mozilla.org/r/192692/#review197872

::: build/moz.configure/toolchain.configure:1287
(Diff revision 1)
>  option('--enable-gold',
>         env='MOZ_FORCE_GOLD',
>         help='Enable GNU Gold Linker when it is not already the default',
>         when=is_linker_option_enabled)
>  
> -imply_option('--enable-linker', 'gold', when='--enable-gold')
> +# This simple wrapper is needed so --enable-gold is registered with help.

Please file a separate bug for this rather than try to fix it here. Because there are two issues:
- the fact that configure is broken, which I don't agree this is the right fix for (specifically, that "when" should count as the option being used)
- the fact that we didn't catch it from unit tests, meaning we should add one.
Attachment #8921682 - Flags: review?(mh+mozilla)
Depends on: 1411462
Comment on attachment 8921683 [details]
Bug 1411081 - Foster debugging of old-configure.vars;

https://reviewboard.mozilla.org/r/192694/#review197874

::: build/moz.configure/old.configure:130
(Diff revision 1)
> -            for key, value in mozconfig['vars']['added'].items():
> +            for key, value in sorted(mozconfig['vars']['added'].items()):
>                  inject("%s=%s" % (key, quote(value)))
> -            for key, (old, value) in mozconfig['vars']['modified'].items():
> +            for key, (old, value) in sorted(mozconfig['vars']['modified'].items()):
>                  inject("%s=%s" % (key, quote(value)))
>              for t in ('env', 'vars'):
> -                for key in mozconfig[t]['removed'].keys():
> +                for key in sorted(mozconfig[t]['removed'].keys()):
>                      inject("unset %s" % key)

This raises the question whether the sorting shouldn't be happening in MozconfigLoader.read_mozconfig.

OTOH, to really make debugging easier, it seems at least added and modified should be sorted/mixed together.
Attachment #8921683 - Flags: review?(mh+mozilla)
Comment on attachment 8921684 [details]
Bug 1411081 - Move --enable-optimize/MOZ_OPTIMIZE to moz.configure;

https://reviewboard.mozilla.org/r/192696/#review197876

::: commit-message-e31ae:9
(Diff revision 1)
> +flag with an optional value. We convert this option to a namespace
> +with a numeric level and string of compiler flags.

I'm not particularly convinced by the added value of changing the semantics of the option. Especially since you don't use it to influence the optimization level for rust (and I don't think we should do that anyways). I'd rather leave the option work the same as it currently does. If you really feel strongly about this, that's still scope bloat, please file a separate bug.

::: js/src/old-configure.in:1440
(Diff revision 1)
> -MOZ_ARG_ENABLE_STRING(optimize,
> -[  --disable-optimize      Disable compiler optimization
> -  --enable-optimize=[OPT] Specify compiler optimization flags [OPT=-O]],
> -[ if test "$enableval" != "no"; then
> +# Use value from moz.configure if one is defined. Else use our computed
> +# value.
> +if test -n "${MOZ_CONFIGURE_OPTIMIZE_FLAGS}"; then
> +    MOZ_OPTIMIZE_FLAGS=${MOZ_CONFIGURE_OPTIMIZE_FLAGS}

You don't need this if you set MOZ_OPTIMIZE_FLAGS from moz.configure, instead of setting MOZ_CONFIGURE_OPTIMIZE_FLAGS. I don't see a reason to use a separate variable here.

::: moz.configure:103
(Diff revision 1)
>  
> +# Code optimization is enabled by default.
> +# unset: no optimization
> +# 1: basic optimization
> +# 2: more optimization
> +js_option('--enable-optimize',

While --enable-debug has a meaning for non-compiled code, --enable-optimize doesn't. Please move this to all to toolchain.configure.

::: moz.configure:109
(Diff revision 1)
> +          nargs='?',
> +          default='1',
> +          help='Compiler optimization level (1 or 2) or flags')
> +
> +@depends('--enable-optimize')
> +@imports('os')

You're not using this import. This should fail the lint test.

::: moz.configure:112
(Diff revision 1)
> +
> +@depends('--enable-optimize')
> +@imports('os')
> +def moz_optimize(option):
> +    # The option can be a PositiveOptionValue, NegativeOptionValue, or
> +    # a regular Option. This makes value comparisons somewhat complicated.

there is no "regular Option"

::: old-configure.in:3716
(Diff revision 1)
> -MOZ_ARG_ENABLE_STRING(optimize,
> -[  --disable-optimize      Disable compiler optimization
> -  --enable-optimize=[OPT] Specify compiler optimization flags [OPT=-O]],
> -[ if test "$enableval" != "no"; then
> +# Use value from moz.configure if one is defined. Else use our computed
> +# value.
> +if test -n "${MOZ_CONFIGURE_OPTIMIZE_FLAGS}"; then
> +    MOZ_OPTIMIZE_FLAGS=${MOZ_CONFIGURE_OPTIMIZE_FLAGS}

same as js/src/old-configure.in.
Attachment #8921684 - Flags: review?(mh+mozilla)
Comment on attachment 8921685 [details]
Bug 1411081 - Derive Rust compiler flags in configure;

https://reviewboard.mozilla.org/r/192698/#review197878

::: build/moz.configure/toolchain.configure:1275
(Diff revision 1)
>  set_config('DEVELOPER_OPTIONS', developer_options)
>  
> +# Rust compiler flags
> +# ===================
> +
> +@depends(moz_optimize, debug_rust, '--enable-debug-symbols')

gah, why is debug_rust in top-level moz.configure?

::: build/moz.configure/toolchain.configure:1276
(Diff revision 1)
> +@checking('for Rust compiler flags',
> +          callback=lambda x: ' '.join(x.flags))

FWIW, I think this is the kind of thing that should not appear in "Checking ..." lines during configure, but in a (currently inexistent) section that's printed out when config.status runs, or at the end of configure, if at all.

::: build/moz.configure/toolchain.configure:1296
(Diff revision 1)
> +    if debug_rust:
> +        opt_level = '1'
> +        debug_assertions = True
> +    else:
> +        if optimize:
> +            opt_level = '2'
> +        else:
> +            opt_level = '0'
> +
> +        debug_assertions = False

That's rather different from what rules.mk does, and I'm not convinced this is better. For instance, you're making --enable-rust-debug always enable optimization at a fixed level.

::: build/moz.configure/toolchain.configure:1309
(Diff revision 1)
> +    elif debug_rust:
> +        debug_info = '1'

not sure we want that.

::: build/moz.configure/toolchain.configure:1320
(Diff revision 1)
> +    return namespace(
> +        opt_level=opt_level,
> +        debug_assertions=debug_assertions,
> +        debug_info=debug_info,
> +        flags=flags,
> +    )

I'd only start returning a namespace if it's actually used.

::: build/moz.configure/toolchain.configure:1328
(Diff revision 1)
> +        debug_info=debug_info,
> +        flags=flags,
> +    )
> +
> +set_config('MOZ_RUST_DEFAULT_FLAGS',
> +           depends(rust_compiler_opts)(lambda x: x.flags))

note: rust_compiler_opts.flags works.
Attachment #8921685 - Flags: review?(mh+mozilla)
Comment on attachment 8921686 [details]
Bug 1411081 - Use opt-level=1 for rustc in default configuration;

https://reviewboard.mozilla.org/r/192700/#review197880

::: build/moz.configure/toolchain.configure:1276
(Diff revision 1)
>  
>  # Rust compiler flags
>  # ===================
>  
> -@depends(moz_optimize, debug_rust, '--enable-debug-symbols')
> +js_option(env='RUSTC_OPT_LEVEL',
> +          nargs=1,

nargs=1 is not necessary

::: build/moz.configure/toolchain.configure:1280
(Diff revision 1)
> -@depends(moz_optimize, debug_rust, '--enable-debug-symbols')
> +js_option(env='RUSTC_OPT_LEVEL',
> +          nargs=1,
> +          help='Rust compiler optimization level (-C opt-level=%s)')
> +
> +@depends('RUSTC_OPT_LEVEL', debug_rust, '--enable-debug-symbols',
> +         '--enable-release', moz_optimize)

Do we want to allow RUST_OPT_LEVEL being set when using --enable-release? It not, we can just do imply_option('RUST_OPT_LEVEL', '1', when='--enable-release')
Attachment #8921686 - Flags: review?(mh+mozilla)
Comment on attachment 8921684 [details]
Bug 1411081 - Move --enable-optimize/MOZ_OPTIMIZE to moz.configure;

https://reviewboard.mozilla.org/r/192696/#review197876

> I'm not particularly convinced by the added value of changing the semantics of the option. Especially since you don't use it to influence the optimization level for rust (and I don't think we should do that anyways). I'd rather leave the option work the same as it currently does. If you really feel strongly about this, that's still scope bloat, please file a separate bug.

Eh? What semantics did I change? I didn't intend to change any semantics.

> You don't need this if you set MOZ_OPTIMIZE_FLAGS from moz.configure, instead of setting MOZ_CONFIGURE_OPTIMIZE_FLAGS. I don't see a reason to use a separate variable here.

We do... because code in old-configure.in blindly sets `MOZ_OPTIMIZE_FLAGS`. I figured it would be easier to use a separate variable than update all code referencing `MOZ_OPTIMIZE_FLAGS`. Yes, the new code is fragile. But at least we shouldn't be touching it again until we move it to moz.configure.
Comment on attachment 8921684 [details]
Bug 1411081 - Move --enable-optimize/MOZ_OPTIMIZE to moz.configure;

https://reviewboard.mozilla.org/r/192696/#review197876

> Eh? What semantics did I change? I didn't intend to change any semantics.

--enable-optimize={1,2} is not a thing currently. MOZ_OPTIMIZE=2 currently means "we were given flags on --enable-optimize", and MOZ_OPTIMIZE=1 means "whatever MOZ_OPTIMIZE_FLAGS says".

> We do... because code in old-configure.in blindly sets `MOZ_OPTIMIZE_FLAGS`. I figured it would be easier to use a separate variable than update all code referencing `MOZ_OPTIMIZE_FLAGS`. Yes, the new code is fragile. But at least we shouldn't be touching it again until we move it to moz.configure.

Oh right, because that MOZ_OPTIMIZE_FLAGS setting above the patch is not the first place we set it. So, maybe remove those blocks above your patch, and set MOZ_CONFIGURE_OPTIMIZE_FLAGS to -O.
Comment on attachment 8921686 [details]
Bug 1411081 - Use opt-level=1 for rustc in default configuration;

https://reviewboard.mozilla.org/r/192700/#review197880

> nargs=1 is not necessary

It does appear to be necessary. At least with the next version of the patch.
Attachment #8921682 - Attachment is obsolete: true
Comment on attachment 8921684 [details]
Bug 1411081 - Move --enable-optimize/MOZ_OPTIMIZE to moz.configure;

https://reviewboard.mozilla.org/r/192696/#review197876

> --enable-optimize={1,2} is not a thing currently. MOZ_OPTIMIZE=2 currently means "we were given flags on --enable-optimize", and MOZ_OPTIMIZE=1 means "whatever MOZ_OPTIMIZE_FLAGS says".

Oh right. That's my jet lagged brain writing code :)

I'll fix this in the next iteration.
Comment on attachment 8921683 [details]
Bug 1411081 - Foster debugging of old-configure.vars;

https://reviewboard.mozilla.org/r/192694/#review197958

::: build/moz.configure/old.configure:138
(Diff revision 2)
> +                if old is None:
> +                    action = 'added'
> +                else:
> +                    action = 'modified from: %s' % old
> +                inject("%s=%s # %s" % (key, quote(value), action))
> +
>              for t in ('env', 'vars'):
> -                for key in mozconfig[t]['removed'].keys():
> -                    inject("unset %s" % key)
> +                for key in sorted(mozconfig[t]['removed'].keys()):
> +                    inject("unset %s # from %s" % (key, t))

I don't really think it matters what the previous values were. You have them in your environment anyways.

::: build/moz.configure/old.configure:155
(Diff revision 2)
>          # Autoconf is special, because it might be passed from
>          # mozconfig['make_extra'], which we don't pass automatically above.
>          inject('export AUTOCONF=%s' % quote(autoconf))
>  
>          for assignment in old_configure_assignments:
> -            inject(assignment)
> +            inject('%s # assignment' % assignment)

Being after # end of mozconfig values is a strong enough indicator already.
Attachment #8921683 - Flags: review?(mh+mozilla)
Comment on attachment 8921720 [details]
Bug 1411081 - Move assignment of default value for MOZ_OPTIMIZE_FLAGS;

https://reviewboard.mozilla.org/r/192726/#review197960
Attachment #8921720 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8921684 [details]
Bug 1411081 - Move --enable-optimize/MOZ_OPTIMIZE to moz.configure;

https://reviewboard.mozilla.org/r/192696/#review197968

::: build/moz.configure/toolchain.configure:26
(Diff revision 3)
> +
> +@depends('--enable-optimize')
> +def moz_optimize(option):
> +    flags = None
> +
> +    if option and len(option):

len(option) is enough.
Attachment #8921684 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8921685 [details]
Bug 1411081 - Derive Rust compiler flags in configure;

https://reviewboard.mozilla.org/r/192698/#review197972

::: build/moz.configure/toolchain.configure:1329
(Diff revision 3)
> +    if not optimize:
> +        opt_level = '0'
> +
> +    # opt-level=0 implies -C debug-assertions, which may not be desired
> +    # unless Rust debugging is enabled.
> +    if opt_level == '0' and not debug_rust:

You could indent one level and just test for not debug_result.
Attachment #8921685 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8921686 [details]
Bug 1411081 - Use opt-level=1 for rustc in default configuration;

https://reviewboard.mozilla.org/r/192700/#review197974

::: build/moz.configure/toolchain.configure:1305
(Diff revision 3)
>  
>  # Rust compiler flags
>  # ==============================================================
>  
> -@depends(moz_optimize, debug_rust, '--enable-debug-symbols')
> -def rust_compiler_flags(moz_optimize, debug_rust, debug_symbols):
> +js_option(env='RUSTC_OPT_LEVEL',
> +          nargs=1,

how about replacing with choices=('1', '2', '0', 's', 'z')?

::: build/moz.configure/toolchain.configure:1336
(Diff revision 3)
> +        if optimize:
> +            opt_level = '1'
> +        else:
> -        opt_level = '0'
> +            opt_level = '0'

opt_level = '1' if optimize else '0'
Attachment #8921686 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8921685 [details]
Bug 1411081 - Derive Rust compiler flags in configure;

https://reviewboard.mozilla.org/r/192698/#review197972

> You could indent one level and just test for not debug_result.

I wrote it this way so the logic is centered around the behavior of that specific value, not a side-effect of build settings. This will ensure that a forced level of 0 (possible in next commit) always does the right thing, regardless of other configure state.
Comment on attachment 8921686 [details]
Bug 1411081 - Use opt-level=1 for rustc in default configuration;

https://reviewboard.mozilla.org/r/192700/#review197974

> how about replacing with choices=('1', '2', '0', 's', 'z')?

I implemented this at one time. However, I think it codes too much implementation details about the Rust compiler. What happens if rustc gains a new value? Should the Firefox build system be need to be taught about this? I think the answer is "no." Instead, configure should validate that the requested option works. Whether we actually do that for rustc (like we do for CC), I'm not sure...
Comment on attachment 8921683 [details]
Bug 1411081 - Foster debugging of old-configure.vars;

https://reviewboard.mozilla.org/r/192694/#review198220

::: commit-message-22ecb:1
(Diff revision 3)
> +Bug 1411081 - Foster debugging of old-configure.vars; r?Build

Do you mean faster?
Comment on attachment 8921683 [details]
Bug 1411081 - Foster debugging of old-configure.vars;

https://reviewboard.mozilla.org/r/192694/#review198220

> Do you mean faster?

No. "Foster" means enable, promote, etc.
Comment on attachment 8921683 [details]
Bug 1411081 - Foster debugging of old-configure.vars;

https://reviewboard.mozilla.org/r/192694/#review198224

This looks sensible.  There's no concern about re-ordering, right, since we already had a dictionary?  Let's find out if subtly depended on order somewhere...
Attachment #8921683 - Flags: review+
Attachment #8921683 - Flags: review?(core-build-config-reviews)
Comment on attachment 8921683 [details]
Bug 1411081 - Foster debugging of old-configure.vars;

https://reviewboard.mozilla.org/r/192694/#review198224

It shouldn't depend on order. The mozconfig evaluation code already puts each variable in exactly one bucket. So there should be no data loss from combining dicts. Furthermore, the source dicts were already unordered. So any re-ordering concerns were already present.
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daedd8936508
Foster debugging of old-configure.vars; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/45ab7c2b4f68
Move assignment of default value for MOZ_OPTIMIZE_FLAGS; r=glandium
https://hg.mozilla.org/integration/autoland/rev/deddd7b55752
Move --enable-optimize/MOZ_OPTIMIZE to moz.configure; r=glandium
https://hg.mozilla.org/integration/autoland/rev/472232499478
Derive Rust compiler flags in configure; r=glandium
https://hg.mozilla.org/integration/autoland/rev/64a852e835b7
Use opt-level=1 for rustc in default configuration; r=glandium
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd0d5dcfe207
Appease flake8 linter by adding empty lines; r=me
Depends on: 1411884
Depends on: 1412044
Blocks: 1412077
See Also: → 1423689
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: