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

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: add a --enable-rust-debug option

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?

Comment 2

[Nathan Froyd [:froydnj]]

Patch posted.

Comment 3

[Mike Hommey [:glandium]]

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 4

[Chris Manchester (limited bugmail, email directly)]

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))`.

Comment 5

[Nathan Froyd [:froydnj]]

(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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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 7

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 8

[Mike Hommey [:glandium]]

(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++.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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.)

Comment 10

[Nathan Froyd [:froydnj]]

(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.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 12

[Nathan Froyd [:froydnj]]

(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.

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: add a --enable-rust-debug option

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d8f0f741be4
add a --enable-rust-debug option; r=chmanchester

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d8f0f741be4