466486 - Don't use a subshell to recurse over DIRS when DIRS is empty

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Benjamin Smedberg]

Attachment: Avoid shells for recursing empty DIRS, rev. 1

While researching build time, I discovered that we're using a subshell to recurse over subdirectories even when there aren't any. This is wasteful, especially on Windows where processes cost a lot.

Comment 1

[(not currently active) Ted Mielczarek]

Comment on attachment 349760 [details] [diff] [review]
Avoid shells for recursing empty DIRS, rev. 1

Do we ever wind up with DIRS set to an empty space? Couldn't we just use |ifdef DIRS| here?

Also, amazing what silliness you can find with a little research!

Comment 2

[Benjamin Smedberg]

I found at least one case with something like:

DIRS += \
  $(EMPTYVALUE) \
  $(NULL)

oh, for make variables that are lists and not scalars ;-)

Comment 3

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/f71446b6fc7e

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

this broke x86_64/linux builds

Comment 5

[Benjamin Smedberg]

backed out... weird regressions with some directories not getting built

Comment 6

[[e-mail]]

Comment on attachment 349760 [details] [diff] [review]
Avoid shells for recursing empty DIRS, rev. 1

>-	@$(EXIT_ON_ERROR) \
>-	$(foreach dir,$(STATIC_DIRS),$(MAKE) -C $(dir); ) true
>+	+$(LOOP_OVER_STATIC_DIRS)

>-ifdef TOOL_DIRS
>-	@$(EXIT_ON_ERROR) \
>-	$(foreach dir,$(TOOL_DIRS),$(UPDATE_TITLE) $(MAKE) -C $(dir) libs; ) true
>-endif
>+	+$(LOOP_OVER_TOOL_DIRS)

Unfortunately $(LOOP_OVER_XXX_DIRS) uses $@ and so these aren't the same...

Comment 7

[Benjamin Smedberg]

Attachment: Avoid shells for recursing empty DIRS, rev. 2

In this version, you aren't allowed to modify DIRS after you've included rules.mk... it would have already caused problems, such as missing submakefile dependencies. Unfortunately I can't think of a good way to enforce this rule, but oh well...

This also fixes misuse of LOOP_OVER_STATIC_DIRS and LOOP_OVER_TOOL_DIRS

Comment 8

[Phil Ringnalda (:philor)]

Looks like both browser/Makefile.in and mail/Makefile.in muck with DIRS after including rules.mk.

Comment 9

[Benjamin Smedberg]

Attachment: Avoid shells for recursing empty DIRS, rev. 2.1

That turns out not to be a problem in practice (because DIRS was already set to something), but I've patched it.

Comment 10

[Benjamin Smedberg]

Second time's the charm: http://hg.mozilla.org/mozilla-central/rev/d11ccfc1d754

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 353702 [details] [diff] [review]
Avoid shells for recursing empty DIRS, rev. 2.1

a191=beltzner

Comment 12

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/758a1804c901