Closed
Bug 428532
Opened 17 years ago
Closed 9 years ago
single compiler invocation for C++ files per dir
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: ted, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 8 obsolete files)
15.35 KB,
patch
|
Details | Diff | Splinter Review |
shaver had a crazy scheme, and I have a patch, which I'll attach when my build finishes. This works on Win32/VC++, might need some tweaking for GCC.
Reporter | ||
Comment 1•17 years ago
|
||
Without: real 27m37.750s With: real 21m51.250s Built without -janything, just a straight |make| on a Core 2 Duo/2gb ram WinXP system.
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•17 years ago
|
||
With -j4, without the patch: real 20m52.047s With -j4, with the patch: real 21m18.469s Makes sense, since we're effectively serializing the C++ compilation phase.
Comment 3•17 years ago
|
||
Yeah, without being able to ||ize dirs, we'll hit a wall there, though I suspect that we use less CPU with the patch even in the -j4 case, so it might still be a win on more-constrained boxes like the VM-hosted tboxes. Nonetheless, I am sad.
Comment 4•17 years ago
|
||
FWIW, I tweaked the patch to work with gcc on Linux. With -j4, without the patch: real 18m With -j4, with the patch: real 30m The PROG_SEPARATE_OBJS change was necessary for xremote: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/xremoteclient/Makefile.in&rev=1.23&root=/cvsroot&mark=74-76#71
Comment 5•17 years ago
|
||
(In reply to comment #4) > With -j4, without the patch: > real 18m > > With -j4, with the patch: > real 30m :(
Reporter | ||
Comment 6•17 years ago
|
||
This would probably be better with bug 167254.
Reporter | ||
Updated•16 years ago
|
Blocks: build-perf
Reporter | ||
Comment 7•16 years ago
|
||
This is the magic+tweaks patch updated to trunk.
Attachment #315144 -
Attachment is obsolete: true
Attachment #315227 -
Attachment is obsolete: true
Reporter | ||
Comment 8•16 years ago
|
||
This is much better with my patch from bug 461395. See the data in bug 461395 comment 1.
Reporter | ||
Comment 9•16 years ago
|
||
Attachment #344514 -
Attachment is obsolete: true
Reporter | ||
Comment 10•16 years ago
|
||
Note to self: I should add something like mddepend.pl does to the deps for $OBJENTRY), so that if any of the object files are missing, it gets rebuilt.
Reporter | ||
Comment 11•16 years ago
|
||
So the data in bug 461395 suggests that this is nice on Windows (with the two patches combined), but break-even or a loss on other platforms. I'm going to tweak this patch so that we'll only enable it on Windows. In fact, the exact logic if going to be something like: if building on Windows: if (makefile requests single-pass-compile) or (building with -j1): enable single-pass-compile Since on Windows it looks to be always a win in the -j1 case, we should enable it there. Otherwise, directories will have to opt-in, since they'll only see a win if they get PARALLEL_DIRS. I'll make content/ use this since it's clearly a win there.
Comment 12•15 years ago
|
||
without this patch, building Fennec for WinMo: real 85m42.050s user 3m9.826s sys 8m33.697s with this patch, building Fennec for WinMo: real 61m52.327s user 2m46.921s sys 7m26.657s ummm....can we land this now? Ted, one note I deleted the line with "OBJENTRY = OBJENTRY.BOGUS" because it was breaking the build.
Attachment #344642 -
Attachment is obsolete: true
Comment 13•15 years ago
|
||
This is simply a better patch. ted made some improvements and I tacked on the use of MSVC9+'s /MP compiler flag. (http://msdn.microsoft.com/en-us/library/bb385193.aspx) Needs the patch from bug 538606 applied as well.
Attachment #402453 -
Attachment is obsolete: true
Attachment #420765 -
Flags: review?(benjamin)
Comment 14•15 years ago
|
||
I had to revert the _VPATH_SRCS changes to fix bustage. It seems to work fine regardless, but I can't vouch for its correctness.
Attachment #420765 -
Attachment is obsolete: true
Attachment #420875 -
Flags: review?(benjamin)
Attachment #420765 -
Flags: review?(benjamin)
Reporter | ||
Comment 15•15 years ago
|
||
Fix a WinCE bug, I missed patching a link command line in the $(PROGRAM) link rule.
Attachment #420875 -
Attachment is obsolete: true
Attachment #421031 -
Flags: review?(benjamin)
Attachment #420875 -
Flags: review?(benjamin)
Reporter | ||
Comment 16•15 years ago
|
||
This is still breaking the WinMo builds on try server (but not the WinCE builds). blassey thinks his patch in bug 533542 might fix it. I've pushed a build with that patch + this patch to the try server to test.
Reporter | ||
Comment 17•15 years ago
|
||
This passed the try server when combined with bug 533542.
Comment 18•15 years ago
|
||
If you have some way of knowing that you're -j1 then there seems to be an easier way of implementing this by adding a rule along these lines: $(TARGETS):: $(CPPSRCS) $(ELOG) $(CCC) -c $(COMPILE_CXXFLAGS) $? early on in the Makefile. (Without -j1 this won't compile the files quickly enough to stop the %.$(OBJ_SUFFIX) rules taking effect.) Either way though, I'm not sure how you expect dependency information to stay up-to-date.
Reporter | ||
Comment 19•15 years ago
|
||
I tried filtering MAKEFLAGS for -j1, but it apparently just doesn't work. I'm not sure what you mean about the dependency information, $(OBJENTRY) depends on $(CPPSRCS).
Comment 20•15 years ago
|
||
(In reply to comment #19) > I tried filtering MAKEFLAGS for -j1, but it apparently just doesn't work. If you use -j1 then MAKEFLAGS won't have any j option at all. (Submakes run with -j and --jobserver-fds.) > I'm not sure what you mean about the dependency information, $(OBJENTRY) > depends on $(CPPSRCS). So what about changes to headers?
Comment 21•14 years ago
|
||
cl's -MP option conflicts with -showIncludes, the subject of bug 518136, so I've dropped the former option.
Attachment #421031 -
Attachment is obsolete: true
Attachment #452267 -
Flags: review?(benjamin)
Attachment #452267 -
Flags: feedback?(ted.mielczarek)
Attachment #421031 -
Flags: review?(benjamin)
Updated•14 years ago
|
Attachment #452267 -
Flags: review?(benjamin)
Attachment #452267 -
Flags: feedback?(ted.mielczarek)
I think we'll end up WONTFIXing this in favor of Bug 623617, but I'll leave this open for now.
Reporter | ||
Updated•13 years ago
|
Assignee: ted.mielczarek → nobody
Comment 23•9 years ago
|
||
I think we can reasonably WONTFIX this, considering bug 939583.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
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
•