The default bug view has changed. See this FAQ.

use PARALLEL_DIRS in netwerk/

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
This doesn't look to be that difficult, and netwerk has quite a number of source files (200+ .cpp).
(Assignee)

Comment 1

5 years ago
Created attachment 654676 [details] [diff] [review]
patch

We can't parallelize everything; build/ has to be built after everything else because it relies on everything else.

Not sure if this needs a necko peer review or not, feel free to bounce it if so.
Attachment #654676 - Flags: review?(mh+mozilla)
(Assignee)

Comment 2

5 years ago
Oh, and this patch wins ~3% of build time on an m-c build (8 core/16 threads Xeon with SSD).

Comment 3

5 years ago
Watch out for etld_data.inc in netwerk/dns. I had to one-off that in build-splendid because of race conditions. I'm not sure if it is just a dependency inside netwerk or whether other parts of the tree don't like when it's missing. If it's other parts of the tree, then PARALLEL_DIRS in netwerk should be safe.
(Assignee)

Comment 4

5 years ago
(In reply to Gregory Szorc [:gps] from comment #3)
> Watch out for etld_data.inc in netwerk/dns. I had to one-off that in
> build-splendid because of race conditions. I'm not sure if it is just a
> dependency inside netwerk or whether other parts of the tree don't like when
> it's missing. If it's other parts of the tree, then PARALLEL_DIRS in netwerk
> should be safe.

Hm, that seems strange.  etld_data.inc is only ever built and touched from within netwerk/dns/.  I haven't seen problems with it in a handful of builds, but that's one of the fun things about parallel programming, isn't it?  Do you happen to recall what the problem was and what you wound up doing about it?
There's no technical reason for netwerk/build to exist anymore, we could actually kill most of it since we don't support linking necko as a standalone library anymore. We still need the nsNetModule bits, but it doesn't have to depend on the other directories. It'd require some fiddling elsewhere to ensure that we link the right bits libxul at the end. We could probably create a netwerk/libs.mk or something that defines all the libraries that we want to link.

Comment 6

5 years ago
My error was probably stemming from how build-splendid does building. Basically, I try to build all the C/CPP files in one pass without looking at the other dependencies in the Makefiles. Looking at the Makefile now, since nsEffectiveTLDService.cpp depends on etld_data.inc, that was breaking build-splendid.

There's only a handful of places where compiling cpp files depends on additional dependencies in Makefiles. If we had a build pass/tier that handled these one-offs, we could convert the *entire* C++ compilation tier to be fully parallel.
We probably just need a generic way to express code generation in our Makefiles.
(Assignee)

Comment 8

5 years ago
(In reply to Ted Mielczarek [:ted] from comment #5)
> There's no technical reason for netwerk/build to exist anymore

Filed bug 785158 for this.

(In reply to Ted Mielczarek [:ted] from comment #7)
> We probably just need a generic way to express code generation in our
> Makefiles.

I think this is not a bad idea, if you can enforce something like "this cpp depends on this file, which is constructed by running this (Python) script with these arguments".  We're doing something similar to etld_data.inc in telemetry in bug 781531 and I cringed just a little bit when I added the build rules to Makefile.in. ;)

Anyway, returning you to your regularly-scheduled netwerk bugs...
Attachment #654676 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → nfroyd
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d6c36e40b8
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/63d6c36e40b8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.