Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla26
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

Assignee

Description

6 years ago
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...
Assignee

Comment 1

6 years ago
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

Updated

6 years ago
Assignee: nobody → gps
Assignee

Comment 2

6 years ago
I combined some duplicate code into a define.
Attachment #791400 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
Attachment #791383 - Attachment is obsolete: true
Attachment #791383 - Flags: review?(mh+mozilla)
Assignee

Comment 3

6 years ago
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)
Assignee

Comment 4

6 years ago
Moved another piece of TIER usage from rules.mk into /Makefile.
Attachment #791424 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
Attachment #791416 - Attachment is obsolete: true
Attachment #791416 - Flags: review?(mh+mozilla)
Assignee

Comment 5

6 years ago
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)
Assignee

Comment 6

6 years ago
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 :)
Assignee

Comment 8

6 years ago
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.
Assignee

Updated

6 years ago
Attachment #791455 - Attachment is obsolete: true
Assignee

Comment 9

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

Comment 10

6 years ago
Fixed typo that failed to write out variables properly.
Attachment #791493 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
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+
Assignee

Comment 14

6 years ago
(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.
Assignee

Comment 15

6 years ago
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
Assignee

Updated

6 years ago
Blocks: 907365
https://hg.mozilla.org/mozilla-central/rev/e6ed18f19d79
https://hg.mozilla.org/mozilla-central/rev/77c09405038a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.