use PARALLEL_DIRS in dom/

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 652529 [details] [diff] [review]
PARALLEL_DIRS magic for dom/

dom/ has grown pretty big, almost as big as content/.  content/ does PARALLEL_DIRS.  dom/ doesn't.  Let's fix that.

Fresh build before PARALLEL_DIRS:

real	10m46.380s
user	75m31.311s
sys	6m47.187s

Fresh build with attached patch:

real	10m9.947s
user	77m9.975s
sys	6m52.453s

so ~5% improvement.

Guessing there are some subtle ordering issues which one run doesn't turn up, though...
Attachment #652529 - Flags: feedback?(khuey)
I'm pretty sure we can't do interfaces/ in parallel.
(Assignee)

Comment 2

5 years ago
Yeah, I do see some random build failures on try.  May look at those today.
(Assignee)

Updated

5 years ago
Attachment #652529 - Flags: feedback?(khuey)
(Assignee)

Comment 3

5 years ago
(In reply to :Ms2ger from comment #1)
> I'm pretty sure we can't do interfaces/ in parallel.

So it *looks* like the only thing preventing this from happening is that, e.g. domstubs.idl is included by a bunch of different files, thus requiring interfaces/base to be built before everything else.  (And other examples, but domstubs is the most prominent one that I've seen.)

dom/interfaces/base/Makefile.in gets around some circularity by appending to XPIDL_FLAGS; would it be reasonable to add the necessary XPIDL_FLAGS+= bits to the appropriate directories so it at least *looks* like we are declaring our dependencies, rather than requiring serialization to sort them out for us?
fiddling with XPIDL_FLAGS (like LOCAL_INCLUDES) is the right way to proceed.
(Assignee)

Comment 5

5 years ago
Created attachment 654617 [details] [diff] [review]
add XPIDL_FLAGS where necessary

Here's the boring patch that adds XPIDL_FLAGS where necessary.  This whole patch was generated by a script, the logic is something like:

for each directory with idl files:
  determine #includes
  find where the #includes live
  write appropriate XPIDL_FLAGS bits to the directory's Makefile.in

so I am reasonably confident it won't produce random build failures.  I assumed that only dependencies in dom/ mattered; other deps should be taken care of by top-level serialization, IIUC.
Attachment #654617 - Flags: review?(khuey)
(Assignee)

Comment 6

5 years ago
Created attachment 654618 [details] [diff] [review]
PARALLEL_DIRS magic for dom/

This patch flips dom/Makefile.in to use PARALLEL_DIRS in all the appropriate places.
Attachment #652529 - Attachment is obsolete: true
Attachment #654618 - Flags: review?(khuey)
(Assignee)

Comment 7

5 years ago
CC'ing gps in case he has insights on this from build-splendid work.
Comment on attachment 654617 [details] [diff] [review]
add XPIDL_FLAGS where necessary

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

XPIDL_FLAGS += \
  directory \
  $(NULL)

Even on the single line ones please.
Attachment #654617 - Flags: review?(khuey) → review+
Comment on attachment 654618 [details] [diff] [review]
PARALLEL_DIRS magic for dom/

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

::: dom/Makefile.in
@@ +43,2 @@
>    interfaces/apps \
>    $(NULL)

Just move this up into the other set of PARALLEL_DIRS.
Attachment #654618 - Flags: review?(khuey) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4219f3723d
https://hg.mozilla.org/integration/mozilla-inbound/rev/544b0cbdbb09
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/fb4219f3723d
https://hg.mozilla.org/mozilla-central/rev/544b0cbdbb09
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Blocks: 786703
So how about we back this out until we can do it right?
Yes, please back this out until we can fix the build issue.
(Assignee)

Comment 14

5 years ago
FWIW, there is a patch for the build issue in bug 786703.
You need to log in before you can comment on or make changes to this bug.