1349694 - only compile gkrust_gtest if we're actually building libxul-gtest

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

With more and more people developing Stylo, Stylo build time is becoming a bigger pain point.  One obvious thing that's within Gecko's control is that we build two static libraries from Rust code:

* gkrust, for linking into libxul;
* gkrust_gtest, for linking into the gtest version of libxul.

We build both of these during normal builds.  We don't build libxul-gtest during normal builds, though, so we're wasting a good bit of time rebuilding gkrust_gtest when we don't need it.

I don't have any non-hacky ideas on how to fix this.  Building libxul-gtest already depends on some Makefile.in hackiness:

http://dxr.mozilla.org/mozilla-central/source/toolkit/library/Makefile.in#15
http://dxr.mozilla.org/mozilla-central/source/toolkit/library/gtest/Makefile.in#17

so the hacky fix is to only declare that we need to build gkrust_gtest when LINK_GTEST_DURING_COMPILE, which requires embedding special knowledge about things in the recursivemake moz.build backend.  We could add a special argument to RustLibrary for "only declare this Rust library when this makefile variable is true", but that might even be more hacky, depending on your taste.

I'm willing to attempt whatever idea we come up with here.

Ted or Chris, do you have any ideas here?

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: only build gkrust-gtest when we build libxul-gtest

To improve build times, we've decided to only build libxul-gtest when
it's specifically requested, i.e. via `mach gtest` or similar.  However,
our build setup specifies that gkrust-gtest, the Rust code specifically
for libxul-gtest, is built during normal builds.  This library takes a
significant amount of time to compile and is generally not needed.

This patch changes the definition of gkrust-gtest Makefile variables to
only be visible under the same conditions that libxul-gtest is built,
thus speeding up normal builds.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8851069 [details] [diff] [review]
only build gkrust-gtest when we build libxul-gtest

Review of attachment 8851069 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1271,5 @@
>          backend_file.write('CARGO_TARGET_DIR := %s\n' % target_dir)
>          if libdef.features:
>              backend_file.write('%s := %s\n' % (libdef.FEATURES_VAR, ' '.join(libdef.features)))
> +        if library_is_gkrust_gtest:
> +            backend_file.write('endif\n')

I see. Is this better than adding a Makefile.in to toolkit/library/gtest/rust and repeating the STANDALONE_MAKEFILE trick from toolkit/library/gtest/Makefile.in ?

Comment 3

[Chris Manchester (limited bugmail, email directly)]

When we asked about disabling building CPPUNITTESTS by default a while back the feedback was that we need to have them build by default so people working in the area will know when something they're doing will break building those tests, even if they're not modifying or running those tests. Is there something about how we're developing stylo that makes this not an issue? We're also not entirely consistent on this point, we don't link the gtest libxul in local builds, although that's much slower.

How much faster is a local build going to be with this patch applied?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> How much faster is a local build going to be with this patch applied?

Depends on the hardware, obviously, but about 2.5 minutes in my case with a 1.5-year-old mbp.  Note that this part of the build is always single-core, so you can't make it better by throwing more cores at it.  Also note that this gets built for every single rust-side stylo change.

For scale, on the same hardware any minor change to the servo side of stylo entails a 7-8 minute rebuild.  2.5 minutes of it is gkrust_text.

Comment 5

[(not currently active) Ted Mielczarek]

I think this is painful enough that froydnj's patch is worth taking, for the same reasons we're not linking xul-gtest. We can see whether this winds up being an issue in practice.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #2)
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +1271,5 @@
> >          backend_file.write('CARGO_TARGET_DIR := %s\n' % target_dir)
> >          if libdef.features:
> >              backend_file.write('%s := %s\n' % (libdef.FEATURES_VAR, ' '.join(libdef.features)))
> > +        if library_is_gkrust_gtest:
> > +            backend_file.write('endif\n')
> 
> I see. Is this better than adding a Makefile.in to
> toolkit/library/gtest/rust and repeating the STANDALONE_MAKEFILE trick from
> toolkit/library/gtest/Makefile.in ?

I'm not sure.  Adding Makefile.in to the tree feels like moving backwards, but maybe if the logic lived in Makefile.in instead of in the recursivemake backend, it'd be easier to fix when the time came?

I'm happy to do this either way.  Your call.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to Chris Manchester (:chmanchester) from comment #2)
> > ::: python/mozbuild/mozbuild/backend/recursivemake.py
> > @@ +1271,5 @@
> > >          backend_file.write('CARGO_TARGET_DIR := %s\n' % target_dir)
> > >          if libdef.features:
> > >              backend_file.write('%s := %s\n' % (libdef.FEATURES_VAR, ' '.join(libdef.features)))
> > > +        if library_is_gkrust_gtest:
> > > +            backend_file.write('endif\n')
> > 
> > I see. Is this better than adding a Makefile.in to
> > toolkit/library/gtest/rust and repeating the STANDALONE_MAKEFILE trick from
> > toolkit/library/gtest/Makefile.in ?
> 
> I'm not sure.  Adding Makefile.in to the tree feels like moving backwards,
> but maybe if the logic lived in Makefile.in instead of in the recursivemake
> backend, it'd be easier to fix when the time came?
> 
> I'm happy to do this either way.  Your call.

Ok, that does seem better assuming it doesn't cause other issues. It does seem like generating ifdefs via the backend is introducing a new flavor of hack we'll have to rip out someday.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: only build gkrust-gtest when we build libxul-gtest

To improve build times, we've decided to only build libxul-gtest when
it's specifically requested, i.e. via `mach gtest` or similar.  However,
our build setup specifies that gkrust-gtest, the Rust code specifically
for libxul-gtest, is built during normal builds.  This library takes a
significant amount of time to compile and is generally not needed.

This patch changes the gkrust-gtest library to only be visible under the
same conditions that libxul-gtest is built, thus speeding up normal
builds.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10e7be66709d
only build gkrust-gtest when we build libxul-gtest; r=chmanchester

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10e7be66709d