462381 - sprinkle magic PARALLEL_DIRS fairy dust about the build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

I landed bug 461395, which adds support for a PARALLEL_DIRS variable to the build system, and uses it in content. We should use this in more places, but it does require a bit of local knowledge, so I'd like to get module owners/peers to handle their own modules.

Using it is incredibly simple. You simply take directories out of the DIRS variable, and instead put them in the PARALLEL_DIRS variable. All directories in PARALLEL_DIRS will have each build pass (export,libs,tools) called on them in parallel if possible.

The trick is that there cannot be dependencies between dirs in the same build pass. You can't have IDL files in one dir including IDL files from another dir if you're going to build them in parallel, and you can't build something like layout/build/ in parallel with the other dirs in layout because that dir links in object files from the other dirs. To get around this, you can use DIRS for some directories and PARALLEL_DIRS for others. All the directories in PARALLEL_DIRS will be built, and then the directories in DIRS will be built serially afterwards.

Comment 1

[Benjamin Smedberg]

Attachment: Build layout in parallel, rev. 1 [Checkin: Comment 4]

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 345532 [details] [diff] [review]
Build layout in parallel, rev. 1
[Checkin: Comment 4]

cool!

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 345532 [details] [diff] [review]
Build layout in parallel, rev. 1
[Checkin: Comment 4]

Nice bonus de-recursification there as well. I should have done that in content, but I just took the easy way out.

Comment 4

[Benjamin Smedberg]

Comment on attachment 345532 [details] [diff] [review]
Build layout in parallel, rev. 1
[Checkin: Comment 4]

Pushed layout: http://hg.mozilla.org/mozilla-central/rev/7aeaf064ad9f

Comment 5

[Mitchell Field [:Mitch]]

Attachment: Build more stuff in parallel

Comment 6

[Simon Montagu :smontagu]

(In reply to comment #0)
> The trick is that there cannot be dependencies between dirs in the same build
> pass. You can't have IDL files in one dir including IDL files from another dir
> if you're going to build them in parallel

Two questions: Is it OK to have .cpp files in one dir including .h files from another dir? What happens if we do a parallel build where this condition isn't met -- will the build just fail or will there be strange and unpredictable results?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 405734 [details] [diff] [review]
Build more stuff in parallel

r=biesi for netwerk/

It's ok if a cpp file in one dir includes an idl-generated .h file from another dir, right?

You could probably get some parallelism in netwerk/ itself too, at least if you can build base/ before all the parallel stuff.

Comment 8

[(not currently active) Ted Mielczarek]

To answer both smontagu and biesi: yes. The export phase is built in parallel, and then the libs phase is built in parallel after that, so inter-dir header/IDL dependencies are fine.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 405734 [details] [diff] [review]
Build more stuff in parallel

r=me for intl

Comment 10

[Benjamin Smedberg]

Comment on attachment 405734 [details] [diff] [review]
Build more stuff in parallel

>diff --git a/browser/Makefile.in b/browser/Makefile.in
>--- a/browser/Makefile.in
>+++ b/browser/Makefile.in
>@@ -37,17 +37,25 @@
> 
> DEPTH		= ..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
> 
> include $(topsrcdir)/config/config.mk
> 
>-DIRS = base components locales themes fuel app
>+PARALLEL_DIRS = \
>+    app \

browser/app must not be built in parallel. In mac builds it is responsible for creating dist/Minefield.app which must not happen until the entire app has been built.

>+    components \
>+    fuel \
>+    locales \
>+    themes \
>+    $(NULL)
>+
>+DIRS = base

I'm confused why browser/base is not parallel, because it looks like it could be. Perhaps you just switched `base` and `app`?

>diff --git a/db/Makefile.in b/db/Makefile.in

> ifndef NSS_DISABLE_DBM
> ifdef MOZ_MORK
>-DIRS		= mdb mork
>+PARALLEL_DIRS = mdb mork
> endif
> endif 
> 
> ifdef MOZ_MORKREADER
>-DIRS		+= morkreader
>+DIRS += morkreader
> endif

Isn't morkreader entirely independent of mdb/mork? Why not build them all in parallel?

r=me with those changes

Comment 11

[Mitchell Field [:Mitch]]

Attachment: Build more stuff in parallel (v2)

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 405848 [details] [diff] [review]
Build more stuff in parallel (v2)

You don't really need my review if you have bsmedberg's, but a few nits:
+PARALLEL_DIRS = \
+    base \

I prefer two-space indent on the continued lines.

diff --git a/nsprpub/config/rules.mk b/nsprpub/config/rules.mk
Make sure to leave this change out of the final patch for checkin.

Comment 13

[Mitchell Field [:Mitch]]

Attachment: Build more stuff in parallel (v3) [Checkin: Comment 14]

Fixed some more dodgy whitespace too.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 405997 [details] [diff] [review]
Build more stuff in parallel (v3)
[Checkin: Comment 14]


http://hg.mozilla.org/mozilla-central/rev/88be36f58e33

Comment 15

[(not currently active) Ted Mielczarek]

File a new bug if you want to do more of this.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

backed out due to MAc build bustage

http://hg.mozilla.org/mozilla-central/rev/351064799c77

Comment 17

[(not currently active) Ted Mielczarek]

Specifically:
/builds/slave/mozilla-central-macosx/build/build/macosx/universal/unify: copyIfIdentical: files differ:
 obj-firefox/ppc/dist/test-package-stage/xpcshell/tests/all-test-dirs.list,
 obj-firefox/i386/dist/test-package-stage/xpcshell/tests/all-test-dirs.list

I think the problem here is that since this patch makes us build more things in parallel, we can build some directories in an unpredictable order. The "all-test-dirs.list" file is generated on the fly during the build, and so the order of entries can wind up different between the PPC and x86 builds. "unify" chokes on that, because it wants them to be the same.

We could fix this in a few ways:
1) Teach unify to special case files like this (Standard8 said he's seen this with comm.manifest before, too)--it'd have to know that some kinds of files are allowed to be different as long as they're the same if you sort all the lines in them
2) Just sort the contents of all-test-dirs.list during "make test-package" or during the unify step.
3) Generate the test manifest in a smarter way, or have it checked into the tree or something.

#2 sounds like the easiest way to go.

Comment 18

[(not currently active) Ted Mielczarek]

(In reply to comment #17)
> 1) Teach unify to special case files like this (Standard8 said he's seen this
> with comm.manifest before, too)--it'd have to know that some kinds of files are
> allowed to be different as long as they're the same if you sort all the lines
> in them

I have a patch for this, it will be on bug 526668 shortly.

Comment 19

[Mitchell Field [:Mitch]]

Attachment: Build more stuff in parallel (v4)

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 412135 [details] [diff] [review]
Build more stuff in parallel (v4)

--- a/browser/Makefile.in
-DEPTH		= ..
-topsrcdir	= @top_srcdir@
-srcdir		= @srcdir@
-VPATH		= @srcdir@
+DEPTH = ..
+topsrcdir = @top_srcdir@
+srcdir = @srcdir@
+VPATH = @srcdir@

I kind of like having the equals signs lined up here, even if you're removing hard tabs.

--- a/content/media/Makefile.in
 DIRS =

You can just ditch this assignment, it doesn't do anything.

 ifdef ENABLE_TESTS
 DIRS += test
 endif

This could be PARALLEL_DIRS as well. There's no linking going on in tests/, just Mochitests.

--- a/netwerk/Makefile.in

You didn't make anything PARALLEL in this makefile. Why not? Aside from build and test, is there any linking happening in other dirs?

--- a/toolkit/mozapps/Makefile.in
Same here, is there no parallelization to be had?
Also, this is a good candidate for your "remove excess recursion" bug, since this Makefile contains nothing but DIRS. Could just move those all up to toolkit/Makefile.in as DIRS += mozapps/foo.

r+ with those addressed. You don't have to fix the places I just asked about adding more parallelism, you could do that in a followup.

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 412135 [details] [diff] [review]
Build more stuff in parallel (v4)

This patch seems to shave a little bit of time (maybe 30s) off of my -j8 clobber build on Linux.

Comment 22

[Mitchell Field [:Mitch]]

Attachment: Build more stuff in parallel (v5)

Comment 23

[(not currently active) Ted Mielczarek]

Pushed the v5 patch to m-c:
http://hg.mozilla.org/mozilla-central/rev/fae81b8a5648

I'm going to close this bug. If you want to do more of this you can file a followup.

Comment 24

[(not currently active) Ted Mielczarek]

I pushed a bustage fix:
http://hg.mozilla.org/mozilla-central/rev/827d8651799e

static-analysis-bsmedberg was burning because it's building non-libxul, and intl/uconv depends on a static library from intl/unicharutil.