428532 - single compiler invocation for C++ files per dir

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: magic

Without:
real    27m37.750s

With:
real    21m51.250s

Built without -janything, just a straight |make| on a Core 2 Duo/2gb ram WinXP system.

Comment 2

[(not currently active) Ted Mielczarek]

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

[Mike Shaver (:shaver emeritus)]

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

[Mats Palmgren (inactive)]

Attachment: magic + tweaks for GCC

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

[Mike Shaver (:shaver emeritus)]

(In reply to comment #4)
> With -j4, without the patch:
> real    18m
> 
> With -j4, with the patch:
> real    30m

:(

Comment 6

[(not currently active) Ted Mielczarek]

This would probably be better with bug 167254.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: magic, updated to mozilla-central tip

This is the magic+tweaks patch updated to trunk.

Comment 8

[(not currently active) Ted Mielczarek]

This is much better with my patch from bug 461395. See the data in bug 461395 comment 1.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: some slight fixes

Comment 10

[(not currently active) Ted Mielczarek]

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.

Comment 11

[(not currently active) Ted Mielczarek]

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

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated patch

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.

Comment 13

[Mitchell Field [:Mitch]]

Attachment: Patch v2

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.

Comment 14

[Mitchell Field [:Mitch]]

Attachment: Patch v2.1

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.

Comment 15

[(not currently active) Ted Mielczarek]

Attachment: Patch v2.2

Fix a WinCE bug, I missed patching a link command line in the $(PROGRAM) link rule.

Comment 16

[(not currently active) Ted Mielczarek]

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.

Comment 17

[(not currently active) Ted Mielczarek]

This passed the try server when combined with bug 533542.

Comment 18

[[e-mail]]

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.

Comment 19

[(not currently active) Ted Mielczarek]

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

[[e-mail]]

(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

[Mitchell Field [:Mitch]]

Attachment: Patch v2.3

cl's -MP option conflicts with -showIncludes, the subject of bug 518136, so I've dropped the former option.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think we'll end up WONTFIXing this in favor of Bug 623617, but I'll leave this open for now.

Comment 23

[Mike Hommey [:glandium]]

I think we can reasonably WONTFIX this, considering bug 939583.