Closed Bug 1353810 Opened 8 years ago Closed 8 years ago

Have a way to build stylo with opt gecko and debug servo

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [stylo])

Attachments

(1 file, 1 obsolete file)

The basic problem is that opt servo builds have horrible build times when built as part of stylo (8+ mins for any change you might name, all single-core). But debug Gecko builds have horrible runtime (multiple minutes to just start the browser and run any test). It would be nice if we could do debug servo + opt gecko to get a build that can actually be built in finite time and then used for local testing.
Flags: needinfo?(nfroyd)
Attached patch add a --enable-rust-debug option (obsolete) — Splinter Review
For people working on Rust code, compiling in debug mode (Cargo's "dev" profile) is convenient: debug assertions are turned on, optimization is turned off, and parallel compilation inside of rustc itself can be used. These things make the build faster and the debugging experience more pleasant. To obtain that currently, one needs to --enable-debug at the Gecko toplevel, which turns on debug assertions for the entire browser, which makes things run unreasonably slowly. So it would be desirable to be able to turn *off* debug mode for the entirety of the browser, but turn on debug mode for the Rust code only. Hence this added switch, --enable-rust-debug, which does what it suggests and defaults to the value of --enable-debug. For our own sanity and because we judge it a non-existent use case, we do not support --enable-debug --disable-rust-debug. bz, can you try this patch and see if it does what you want?
Attachment #8855039 - Flags: review?(cmanchester)
Attachment #8855039 - Flags: feedback?(bzbarsky)
Patch posted.
Flags: needinfo?(nfroyd)
I'm not convinced an opt-in flag is very nice for developers. Does disabling lto make enough of a difference? if so, we could disable lto on local builds (but enable it automatically on --enable-release builds)
Comment on attachment 8855039 [details] [diff] [review] add a --enable-rust-debug option Review of attachment 8855039 [details] [diff] [review]: ----------------------------------------------------------------- I'm interested in Mike's question... it sounds like we end up with a default configuration that's not reasonable to use for development even with this patch. ::: moz.configure @@ +112,5 @@ > + # Our life is much easier if --enable-debug can't be overriden by > + # --disable-rust-debug. Wanting to build with debug assertions > + # everywhere but not in Rust code is a peculiar choice in any event, > + # given the relative ratios of Rust and C++ code at this time. > + if rust_only_debug.origin != 'default' and not rust_only_debug: If rust_only_debug is true it's not from the default, the origin check doesn't seem particularly meaningful. This should probably be implemented in terms of `imply_option`. something like `imply_option('--enable-rust-debug', depends('--enable-debug')(lambda v: bool(v) or None))`.
Attachment #8855039 - Flags: review?(cmanchester)
(In reply to Mike Hommey [:glandium] from comment #3) > I'm not convinced an opt-in flag is very nice for developers. > Does disabling lto make enough of a difference? if so, we could disable lto > on local builds (but enable it automatically on --enable-release builds) I don't know. We could try that, I guess. We'd probably also have to set up a higher value of codegen-units, too. > ::: moz.configure > @@ +112,5 @@ > > + # Our life is much easier if --enable-debug can't be overriden by > > + # --disable-rust-debug. Wanting to build with debug assertions > > + # everywhere but not in Rust code is a peculiar choice in any event, > > + # given the relative ratios of Rust and C++ code at this time. > > + if rust_only_debug.origin != 'default' and not rust_only_debug: > > If rust_only_debug is true it's not from the default, the origin check > doesn't seem particularly meaningful. This isn't checking for rust_only_debug being True, it's checking for it being False, which is the default value. > This should probably be implemented in terms of `imply_option`. something > like `imply_option('--enable-rust-debug', depends('--enable-debug')(lambda > v: bool(v) or None))`. This doesn't work, because then you can't specify --enable-rust-debug --disable-debug.
> I'm not convinced an opt-in flag is very nice for developers. There's nothing nice for developers here, sadly. Either you get "opt" builds that perform worse than nightlies for no really good reason or you get sadfaces incremental rebuild times. Maybe the right answer is to default to more than one codegen unit for opt builds. That might not hurt perf too much. Hard to say until we try. And at the very least that would give us _some_ parallelism in the build.
Comment on attachment 8855039 [details] [diff] [review] add a --enable-rust-debug option This fails to build because it's looking for gkrust in the wrong place: gmake[5]: *** No rule to make target '../../toolkit/library/rust/../x86_64-apple-darwin/release/libgkrust.a', needed by 'XUL'. Stop. libgkrust.a is of course in ../../toolkit/library/rust/../x86_64-apple-darwin/debug/libgkrust.a But ok, it's building debug servo bits. ;)
Attachment #8855039 - Flags: feedback?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6) > > I'm not convinced an opt-in flag is very nice for developers. > > There's nothing nice for developers here, sadly. Either you get "opt" > builds that perform worse than nightlies for no really good reason or you > get sadfaces incremental rebuild times. You already get opt builds that perform worse than nightlies for C++.
> You already get opt builds that perform worse than nightlies for C++. That's true, but I was led to believe the difference for Rust was much larger. (Also, is that true on Mac? I thought we didn't do LTO for nightlies there.)
Assignee: nobody → nfroyd
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #9) > > You already get opt builds that perform worse than nightlies for C++. > > That's true, but I was led to believe the difference for Rust was much > larger. > > (Also, is that true on Mac? I thought we didn't do LTO for nightlies there.) We do not do any kind of LTO on Mac for our C++ code. We turn on LTO for our Rust code because it's the only way to make the Rust compiler do what we want wrt symbol visibility and dead code stripping...and of course it has other benefits as well. (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #6) > Maybe the right answer is to default to more than one codegen unit for opt > builds. That might not hurt perf too much. Hard to say until we try. And > at the very least that would give us _some_ parallelism in the build. We have to turn off LTO to use multiple codegen units. =/ So our options are: * Some kind of --enable-rust-debug option that people have to manually enable. This has the virtue of actually getting debug Rust (i.e. -O0, assertions enabled). * Disable LTO on Rust code and set codegen-units to 4 or so by default on opt builds, and have --enable-release enable LTO. This doesn't address debuggable Rust code, but it would make opt builds build somewhat faster, presumably. I don't think these are mutually exclusive.
(In reply to Nathan Froyd [:froydnj] from comment #5) > (In reply to Mike Hommey [:glandium] from comment #3) > > I'm not convinced an opt-in flag is very nice for developers. > > Does disabling lto make enough of a difference? if so, we could disable lto > > on local builds (but enable it automatically on --enable-release builds) > > I don't know. We could try that, I guess. We'd probably also have to set > up a higher value of codegen-units, too. > > > ::: moz.configure > > @@ +112,5 @@ > > > + # Our life is much easier if --enable-debug can't be overriden by > > > + # --disable-rust-debug. Wanting to build with debug assertions > > > + # everywhere but not in Rust code is a peculiar choice in any event, > > > + # given the relative ratios of Rust and C++ code at this time. > > > + if rust_only_debug.origin != 'default' and not rust_only_debug: > > > > If rust_only_debug is true it's not from the default, the origin check > > doesn't seem particularly meaningful. > > This isn't checking for rust_only_debug being True, it's checking for it > being False, which is the default value. Ok, I see. > > > This should probably be implemented in terms of `imply_option`. something > > like `imply_option('--enable-rust-debug', depends('--enable-debug')(lambda > > v: bool(v) or None))`. > > This doesn't work, because then you can't specify --enable-rust-debug > --disable-debug. I think this will work.
(In reply to Chris Manchester (:chmanchester) from comment #11) > > > This should probably be implemented in terms of `imply_option`. something > > > like `imply_option('--enable-rust-debug', depends('--enable-debug')(lambda > > > v: bool(v) or None))`. > > > > This doesn't work, because then you can't specify --enable-rust-debug > > --disable-debug. > > I think this will work. Ah, I was getting confused before because I was trying --enable-debug --disable-rust-debug, which I actually disallowed in my patch! So the imply_option is the way to go, even if the error message you get would be confusing. Second wrinkle: we need some source code changes: /home/froydnj/src/gecko-dev.git/xpcom/rust/nsstring/src/lib.rs:426: error: undefined reference to 'Gecko_IncrementStringAdoptCount' /home/froydnj/src/gecko-dev.git/xpcom/rust/nsstring/src/lib.rs:426: error: undefined reference to 'Gecko_IncrementStringAdoptCount' /home/froydnj/src/gecko-dev.git/servo/components/style/gecko/wrapper.rs:230: error: undefined reference to 'Gecko_FlattenedTreeParentIsParent' /home/froydnj/src/gecko-dev.git/servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs:425: error: undefined reference to 'Gecko_IncrementStringAdoptCount' /home/froydnj/src/gecko-dev.git/servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs:425: error: undefined reference to 'Gecko_IncrementStringAdoptCount' For Gecko_IncrementStringAdoptCount: http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsSubstring.cpp#386 we can just use the current version for DEBUG and a no-op stub for MOZ_DEBUG_RUST, since we wouldn't have all the DEBUG-only refcounting machinery available. For Gecko_FlattenedTreeParentIsParent: http://dxr.mozilla.org/mozilla-central/source/layout/style/ServoBindings.cpp#86 I think it's safe to define that for MOZ_DEBUG_RUST always; the called code in nsIContentInlines.h does not seem to be DEBUG-only. But this means that changes can easily break --disable-debug --enable-rust-debug builds. Maybe that's a chance we're willing to take if it makes things move faster.
Let's try this again; I think this patch is separate from the speed of opt builds, as comment 10 suggests. We can try to address that in another bug, if we like.
Attachment #8856979 - Flags: review?(cmanchester)
Attachment #8855039 - Attachment is obsolete: true
Attachment #8856979 - Flags: review?(cmanchester) → review+
Whiteboard: [stylo]
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d8f0f741be4 add a --enable-rust-debug option; r=chmanchester
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: