Closed Bug 603214 Opened 9 years ago Closed 9 years ago

Build NSPR in parallel

Categories

(NSPR :: NSPR, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: khuey, Assigned: khuey)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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)
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).
Attached patch Patch w/comment (obsolete) — Splinter Review
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)
Attached patch Patch v3 by Kyle Huey (obsolete) — Splinter Review
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)
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.
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)
Status: ASSIGNED → RESOLVED
Closed: 9 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.