Closed Bug 906101 Opened 7 years ago Closed 7 years ago

Make tiers less tier-y

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(5 files, 4 obsolete files)

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...
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.
Attachment #791383 - Flags: review?(mh+mozilla)
Assignee: nobody → gps
I combined some duplicate code into a define.
Attachment #791400 - Flags: review?(mh+mozilla)
Attachment #791383 - Attachment is obsolete: true
Attachment #791383 - Flags: review?(mh+mozilla)
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.
Attachment #791416 - Flags: review?(mh+mozilla)
Moved another piece of TIER usage from rules.mk into /Makefile.
Attachment #791424 - Flags: review?(mh+mozilla)
Attachment #791416 - Attachment is obsolete: true
Attachment #791416 - Flags: review?(mh+mozilla)
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."
Attachment #791427 - Flags: review?(mh+mozilla)
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 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 :)
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.
Attachment #791455 - Attachment is obsolete: true
I lied in the last attachment comments: this is the patch that extracts
the export subtier into the new splendid tier.
Fixed typo that failed to write out variables properly.
Attachment #791493 - Flags: review?(mh+mozilla)
Attachment #791427 - Attachment is obsolete: true
Attachment #791427 - Flags: review?(mh+mozilla)
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 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.
Attachment #791400 - Flags: review?(mh+mozilla) → review+
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.
Attachment #791424 - Flags: review?(mh+mozilla) → review+
Attachment #791493 - Flags: review?(mh+mozilla) → review+
(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.
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.
Status: NEW → ASSIGNED
Blocks: 907365
https://hg.mozilla.org/mozilla-central/rev/e6ed18f19d79
https://hg.mozilla.org/mozilla-central/rev/77c09405038a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.