Build NSPR in parallel

RESOLVED FIXED in 4.8.7

Status

NSPR
NSPR
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 482156 [details] [diff] [review]
Patch

This patch drops the time to build NSPR on my system from 35s to 7.5s (500% increase in speed).  The only complex target I saw is nspr4, if my eyes aren't deceiving me all the rest only use object files from one directory.
Attachment #482156 - Flags: superreview?(wtc)
Attachment #482156 - Flags: review?(ted.mielczarek)

Comment 1

8 years ago
khuey: Thanks for the patch.  Please add a comment to explain why adding a 'build' makefile target fixes the problem.

+export::
+	$(MAKE) -C . build

You should be able to omit "-C .", right?

Can you write this makefile rule as follows?

export:: build
No.  In a parallel make, all of the dependencies of a target are executed concurrently (up to the N maximum jobs from -jN).  The instructions for a target, are however, executed sequentially.  By moving the final linking of nspr4 to an additional target we can ensure that final linking occurs after we loop through the other directories (since rules.mk gets included before this).
Created attachment 482702 [details] [diff] [review]
Patch w/comment
Attachment #482156 - Attachment is obsolete: true
Attachment #482702 - Flags: superreview?(wtc)
Attachment #482702 - Flags: review?(ted.mielczarek)
Attachment #482156 - Flags: superreview?(wtc)
Attachment #482156 - Flags: review?(ted.mielczarek)

Comment 4

8 years ago
Created attachment 482736 [details] [diff] [review]
Patch v3 by Kyle Huey

khuey: thanks for the explanation.  I understand it now.
I also compared your fix with the previous attempt in
bug 123423.  Both patches have the same idea: recurse
into $(DIRS) before building $(TARGETS).  I'm glad you
made it work.

I edited your comment and removed "-C .", which I
believe is unnecessary.  Please review this patch.

I think we can fix this in a more elegant way, but
your fix is fine (most bang for the buck).
Attachment #482702 - Attachment is obsolete: true
Attachment #482736 - Flags: superreview?(ted.mielczarek)
Attachment #482736 - Flags: review?(khuey)
Attachment #482702 - Flags: superreview?(wtc)
Attachment #482702 - Flags: review?(ted.mielczarek)

Comment 5

8 years ago
Should I change the comment to say what I summarized in comment 4?
  We use a 'build' target here to ensure that we build $(TARGETS)
  after recursing into $(DIRS) to create the object files in a
  parallel build.  ...

or

  We use a 'build' target here to ensure that we build $(TARGETS)
  after looping over $(DIRS) to create the object files in a
  parallel build.  ...
Comment on attachment 482736 [details] [diff] [review]
Patch v3 by Kyle Huey

I prefer the second comment.
Attachment #482736 - Flags: review?(khuey) → review+
(In reply to comment #5)
>   We use a 'build' target here to ensure that we build $(TARGETS)
>   after looping over $(DIRS) to create the object files in a
>   parallel build.  ...

This one.

Comment 8

8 years ago
Created attachment 482741 [details] [diff] [review]
Patch v4 by Kyle Huey

Patch checked in on the NSPR trunk (NSPR 4.8.7).

Checking in Makefile.in;
/cvsroot/mozilla/nsprpub/Makefile.in,v  <--  Makefile.in
new revision: 1.25; previous revision: 1.24
done
Checking in pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.60; previous revision: 1.59
done
Attachment #482736 - Attachment is obsolete: true
Attachment #482736 - Flags: superreview?(ted.mielczarek)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8.7
You need to log in before you can comment on or make changes to this bug.