Closed Bug 428532 Opened 14 years ago Closed 7 years ago

single compiler invocation for C++ files per dir

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ted, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

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.
Attached patch magic (obsolete) — Splinter Review
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
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.
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.
Attached patch magic + tweaks for GCC (obsolete) — Splinter Review
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
(In reply to comment #4)
> With -j4, without the patch:
> real    18m
> 
> With -j4, with the patch:
> real    30m

:(
This would probably be better with bug 167254.
This is the magic+tweaks patch updated to trunk.
Attachment #315144 - Attachment is obsolete: true
Attachment #315227 - Attachment is obsolete: true
This is much better with my patch from bug 461395. See the data in bug 461395 comment 1.
Attached patch some slight fixes (obsolete) — Splinter Review
Attachment #344514 - Attachment is obsolete: true
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.
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.
Attached patch updated patch (obsolete) — Splinter Review
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
Depends on: 538606
Attached patch Patch v2 (obsolete) — Splinter Review
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)
Attached patch Patch v2.1 (obsolete) — Splinter Review
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)
Attached patch Patch v2.2 (obsolete) — Splinter Review
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)
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.
Depends on: 533542
This passed the try server when combined with bug 533542.
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.
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).
(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?
Attached patch Patch v2.3Splinter Review
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)
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.
Assignee: ted.mielczarek → nobody
I think we can reasonably WONTFIX this, considering bug 939583.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.