906101 - Make tiers less tier-y

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Tier traversal in the build system is an ugly hack to work around missing dependencies. Our current tier traversal mechanism is suboptimal because we have a lot of tiers and subtiers executing sequentially when they could be executing concurrently. I propose blowing up the existing tier traversal mechanism and reassembling it as something that's much more efficient. This should result in some nice build time wins.

I started a quick patch last night. Will hopefully post something in the next few hours...

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Make tier traversal logic more palpable

Definition of tier rules was being performed in target_<subtier>.mk
files and was decoupled from the rest of the tier logic in rules.mk.
Logic has now been consolidated into rules.mk and target_export.mk and
target_tools.mk have been removed because they are now empty or
contained so few statements they were practically worthless.

This patch sets the stage for subsequent tier logic refactoring.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 1: Make tier traversal logic more palpable

I combined some duplicate code into a define.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 1b: Move tier rule into own .mk file

TIERS is only used in the root Makefile. It's silly to have this code in
rules.mk. So I simply moved things out of rules.mk and into a
standalone.mk and included it in /Makefile.

I plan to merge this patch into the previous patch before commit. Just
figured it would be easier grok diffs as multiple patches.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 1b: Move tier rule into own .mk file

Moved another piece of TIER usage from rules.mk into /Makefile.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 1c: Munge DIRS for tiers in recursive make backend

rules.mk contained logic for integrating tier directories into DIRS.
This doesn't belong in rules.mk because TIERS is only used by the root
Makefile. I moved the munging into backend.mk.

Eventually, lots of this tiers logic in moz.build will change. Until
then, this keeps rules.mk "pure."

Comment 6

[Gregory Szorc [:gps]]

Attachment: Part 2: Build many components concurrently

The nspr, nss, js, external, and precompile tiers have been collapsed
into a single tier and are now built as subtiers. Each subtier has
proper dependencies to ensure maximum concurrency.

With this patch, we can build ipdl, webidl, xpidl, external, and js
concurrently. It's pretty amazing seeing mach's status reporter have
so many active pieces in parallel.

There is more work to be done on this, so not requesting review at this
time. I have more optimizations in the works, including making the
export subtiers concurrent.

https://tbpl.mozilla.org/?tree=Try&rev=0a6a09b145c8

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 791424 [details] [diff] [review]
Part 1b: Move tier rule into own .mk file

>+define CREATE_SUBTIER_RULE
>+$(2)_tier_$(1):
>+	$$(call BUILDSTATUS,SUBTIER_START  $(1) $(2) $$(tier_$(1)_dirs))
>+	$$(foreach dir,$$(tier_$(1)_dirs),$$(call TIER_DIR_SUBMAKE,$(1),$(2),$$(dir),$(2)))

seems like a comment of what args are might be nicer than having to figure it out each time from what they're used for.

>+	$$(call BUILDSTATUS,SUBTIER_FINISH $(1) $(2))
>+
>+endef

nit, isn't that blank line weird?

>+
>+# This function is called and evaluated to produce the rule to build the
>+# specified tier.
>+#
>+# Tiers are traditionally composed of directories that are invoked either
>+# once (so-called "static" directories) or 3 times with the export, libs, and
>+# tools sub-tiers.
>+#
>+# If the TIER_$(tier)_CUSTOM variable is defined, then these traditional
>+# tier rules are ignored and each directory in the tier is executed via a
>+# sub-make invocation (make -C).
>+define CREATE_TIER_RULE

same comment about args :)

Comment 8

[Gregory Szorc [:gps]]

Attachment: Part 2: Build many components concurrently

Now with the base tier folded into the splendid tier and the platform
and app export subtiers also extracted into the splendid tier. It's
kinda absurd how much goes on in parallel with this patch applied.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Part 2b: Extract export subtier into splendid tier

I lied in the last attachment comments: this is the patch that extracts
the export subtier into the new splendid tier.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Part 1c: Munge DIRS for tiers in recursive make backend

Fixed typo that failed to write out variables properly.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 791490 [details] [diff] [review]
Part 2: Build many components concurrently

Review of attachment 791490 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +104,3 @@
>  
>  include $(topsrcdir)/config/makefiles/tiers.mk
> +include $(topsrcdir)/config/tiers.mk

It seems confusing to have these two files with the same name.

::: config/tiers.mk
@@ +49,5 @@
> +	$(call BUILDSTATUS,SUBTIER_FINISH splendid external)
> +
> +phony_targets += nss
> +ifndef MOZ_NATIVE_NSS
> +nss: base nspr external

nss depends on external?

::: js/src/config/makefiles/tiers.mk
@@ +22,4 @@
>  # once (so-called "static" directories) or 3 times with the export, libs, and
>  # tools sub-tiers.
>  #
>  # If the TIER_$(tier)_CUSTOM variable is defined, then these traditional

Ahem

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 791400 [details] [diff] [review]
Part 1: Make tier traversal logic more palpable

Review of attachment 791400 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ +803,5 @@
> +$(foreach subtier,export libs tools,$(eval $(call CREATE_SUBTIER_TRAVERSAL_RULE,$(subtier))))
> +
> +export:: $(SUBMAKEFILES) $(MAKE_DIRS)
> +	$(LOOP_OVER_DIRS)
> +	$(LOOP_OVER_TOOL_DIRS)

Is there actually a tool dir where we do something for export?

@@ +808,5 @@
> +
> +
> +tools:: $(SUBMAKEFILES) $(MAKE_DIRS)
> +	$(LOOP_OVER_DIRS)
> +ifneq (,$(strip $(TOOL_DIRS)))

Considering the foreach is not doing to do anything if TOOL_DIRS is empty, you can remove this condition.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 791424 [details] [diff] [review]
Part 1b: Move tier rule into own .mk file

Review of attachment 791424 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/makefiles/tiers.mk
@@ +29,5 @@
> +tier_$(1)::
> +ifdef TIER_$(1)_CUSTOM
> +	$$(foreach dir,$$($$@_dirs),$$(call SUBMAKE,,$$(dir)))
> +else
> +	$(call BUILDSTATUS_P,BUILDSTATUS TIER_START $(1))

This is obviously on top of bug 905879, please make sure to adjust after applying my review comments there.

Comment 14

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #12)
> ::: config/rules.mk
> @@ +803,5 @@
> > +$(foreach subtier,export libs tools,$(eval $(call CREATE_SUBTIER_TRAVERSAL_RULE,$(subtier))))
> > +
> > +export:: $(SUBMAKEFILES) $(MAKE_DIRS)
> > +	$(LOOP_OVER_DIRS)
> > +	$(LOOP_OVER_TOOL_DIRS)
> 
> Is there actually a tool dir where we do something for export?

I think this is followup territory, possibly in part 2.

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ed18f19d79

Merged part 1 and landed. I'm going to clone this bug to track the part 2 work since I suspect part 2 could linger for a while and I don't like lingering [leave open] bugs.

Comment 16

[Mike Hommey [:glandium]]

Fixup
https://hg.mozilla.org/integration/mozilla-inbound/rev/77c09405038a

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e6ed18f19d79
https://hg.mozilla.org/mozilla-central/rev/77c09405038a