sprinkle magic PARALLEL_DIRS fairy dust about the build system

RESOLVED FIXED

Status

defect
RESOLVED FIXED
11 years ago
Last year

People

(Reporter: ted, Assigned: Mitch)

Tracking

(Blocks 1 bug)

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
Attachment #345532 - Flags: superreview?(roc)
Attachment #345532 - Flags: review?(ted.mielczarek)
Comment on attachment 345532 [details] [diff] [review]
Build layout in parallel, rev. 1
[Checkin: Comment 4]

cool!
Attachment #345532 - Flags: superreview?(roc) → superreview+
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.
Attachment #345532 - Flags: review?(ted.mielczarek) → review+
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
Attachment #345532 - Attachment description: Build layout in parallel, rev. 1 → Build layout in parallel, rev. 1 (pushed)
Depends on: 488091
Posted patch Build more stuff in parallel (obsolete) — Splinter Review
Attachment #405734 - Flags: review?(ted.mielczarek)
Attachment #405734 - Flags: review?(smontagu)
Attachment #405734 - Flags: review?(benjamin)
Attachment #405734 - Flags: review?(cbiesinger)
(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 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.
Attachment #405734 - Flags: review?(cbiesinger) → review+
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 on attachment 405734 [details] [diff] [review]
Build more stuff in parallel

r=me for intl
Attachment #405734 - Flags: review?(smontagu) → review+
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
Attachment #405734 - Flags: review?(benjamin) → review+
Attachment #405734 - Attachment is obsolete: true
Attachment #405848 - Flags: review?(ted.mielczarek)
Attachment #405734 - Flags: review?(ted.mielczarek)
Attachment #405848 - Flags: review?(ted.mielczarek) → review+
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.
Assignee: nobody → mitch_1_2
Fixed some more dodgy whitespace too.
Attachment #405848 - Attachment is obsolete: true
Comment on attachment 405997 [details] [diff] [review]
Build more stuff in parallel (v3)
[Checkin: Comment 14]


http://hg.mozilla.org/mozilla-central/rev/88be36f58e33
Attachment #405997 - Attachment description: Build more stuff in parallel (v3) → Build more stuff in parallel (v3) [Checkin: Comment 14]
Attachment #345532 - Attachment description: Build layout in parallel, rev. 1 (pushed) → Build layout in parallel, rev. 1 [Checkin: Comment 4]
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed
File a new bug if you want to do more of this.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
backed out due to MAc build bustage

http://hg.mozilla.org/mozilla-central/rev/351064799c77
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Depends on: 526668
(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.
Attachment #405997 - Attachment is obsolete: true
Attachment #412135 - Flags: review?(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.
Attachment #412135 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #412135 - Attachment is obsolete: true
Attachment #412304 - Flags: review?(ted.mielczarek)
Attachment #412304 - Flags: review?(ted.mielczarek) → review+
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.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.