sprinkle magic PARALLEL_DIRS fairy dust about the build system

RESOLVED FIXED

Status

RESOLVED FIXED
10 years ago
9 months ago

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.

Comment 1

10 years ago
Created attachment 345532 [details] [diff] [review]
Build layout in parallel, rev. 1
[Checkin: Comment 4]
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+
Blocks: 459891

Comment 4

10 years ago
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
(Assignee)

Comment 5

9 years ago
Created attachment 405734 [details] [diff] [review]
Build more stuff in parallel
Attachment #405734 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

9 years ago
Attachment #405734 - Flags: review?(smontagu)
(Assignee)

Updated

9 years ago
Attachment #405734 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
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+
(Assignee)

Comment 11

9 years ago
Created attachment 405848 [details] [diff] [review]
Build more stuff in parallel (v2)
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
(Assignee)

Comment 13

9 years ago
Created attachment 405997 [details] [diff] [review]
Build more stuff in parallel (v3)
[Checkin: Comment 14]

Fixed some more dodgy whitespace too.
Attachment #405848 - Attachment is obsolete: true
Keywords: checkin-needed
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
Last Resolved: 9 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.
(Assignee)

Comment 19

9 years ago
Created attachment 412135 [details] [diff] [review]
Build more stuff in parallel (v4)
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.
(Assignee)

Comment 22

9 years ago
Created attachment 412304 [details] [diff] [review]
Build more stuff in parallel (v5)
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
Last Resolved: 9 years ago9 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.

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.