Closed
Bug 462381
Opened 16 years ago
Closed 15 years ago
sprinkle magic PARALLEL_DIRS fairy dust about the build system
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: Mitch)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
ted
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
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+
Reporter | ||
Comment 3•16 years ago
|
||
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+
Reporter | ||
Updated•16 years ago
|
Blocks: build-perf
Comment 4•16 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)
Assignee | ||
Updated•15 years ago
|
Attachment #405734 -
Flags: review?(smontagu)
Assignee | ||
Updated•15 years ago
|
Attachment #405734 -
Flags: review?(benjamin)
Assignee | ||
Updated•15 years ago
|
Attachment #405734 -
Flags: review?(cbiesinger)
Comment 6•15 years ago
|
||
(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•15 years ago
|
||
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+
Reporter | ||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
Comment on attachment 405734 [details] [diff] [review] Build more stuff in parallel r=me for intl
Attachment #405734 -
Flags: review?(smontagu) → review+
Comment 10•15 years ago
|
||
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•15 years ago
|
||
Attachment #405734 -
Attachment is obsolete: true
Attachment #405848 -
Flags: review?(ted.mielczarek)
Attachment #405734 -
Flags: review?(ted.mielczarek)
Reporter | ||
Updated•15 years ago
|
Attachment #405848 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 12•15 years ago
|
||
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.
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → mitch_1_2
Assignee | ||
Comment 13•15 years ago
|
||
Fixed some more dodgy whitespace too.
Attachment #405848 -
Attachment is obsolete: true
Reporter | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 14•15 years ago
|
||
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]
Updated•15 years ago
|
Attachment #345532 -
Attachment description: Build layout in parallel, rev. 1 (pushed) → Build layout in parallel, rev. 1
[Checkin: Comment 4]
Updated•15 years ago
|
Reporter | ||
Comment 15•15 years ago
|
||
File a new bug if you want to do more of this.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 16•15 years ago
|
||
backed out due to MAc build bustage http://hg.mozilla.org/mozilla-central/rev/351064799c77
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 17•15 years ago
|
||
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.
Reporter | ||
Comment 18•15 years ago
|
||
(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•15 years ago
|
||
Attachment #405997 -
Attachment is obsolete: true
Attachment #412135 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 20•15 years ago
|
||
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+
Reporter | ||
Comment 21•15 years ago
|
||
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•15 years ago
|
||
Attachment #412135 -
Attachment is obsolete: true
Attachment #412304 -
Flags: review?(ted.mielczarek)
Reporter | ||
Updated•15 years ago
|
Attachment #412304 -
Flags: review?(ted.mielczarek) → review+
Reporter | ||
Comment 23•15 years ago
|
||
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: 15 years ago → 15 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 24•15 years ago
|
||
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•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•