907365 - Make tiers less tier-y, part deux

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #906101 +++

This bug will track collapsing tiers into something less tier-y.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Collapse many build tiers

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.

The code for defining tiers has moved into the root Makefile.

This patch is kinda hacky (we no longer enumerate TIERS which means
projects will need to reimplement TIERS traversal logic). But, it
results in some nice net wins, so I think we should consider landing it.

https://tbpl.mozilla.org/?tree=Try&rev=56ffc7e86bd0

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 793736 [details] [diff] [review]
Collapse many build tiers

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

Too many questions to make it a + or a -.

::: Makefile.in
@@ +97,4 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  default all alldep::
> +	$(call BUILDSTATUS,TIERS splendid platform app)

s/splendid/something else/

@@ +215,5 @@
>  endif
> +
> +define make_subtier_dir
> +$(call BUILDSTATUS,SUBTIER_START splendid $(1))
> ++$(MAKE) -C $(2) $(3)

why not SUBMAKE?

why not also include the .PHONY and the rule?

@@ +232,5 @@
> +ifndef MOZ_NATIVE_NSPR
> +nspr: base
> +	$(call BUILDSTATUS,SUBTIER_START  splendid nspr)
> +	$(call SUBMAKE,,config/nspr)
> +	$(call BUILDSTATUS,SUBTIER_FINISH splendid nspr)

why not make_subtier_dir?

@@ +246,5 @@
> +
> +.PHONY: xpidl
> +xpidl:
> +	$(call BUILDSTATUS,SUBTIER_START  splendid XPIDL)
> +	+$(MAKE) -C $(DEPTH)/xpcom/idl-parser xpidl-parser

SUBMAKE ?

@@ +248,5 @@
> +xpidl:
> +	$(call BUILDSTATUS,SUBTIER_START  splendid XPIDL)
> +	+$(MAKE) -C $(DEPTH)/xpcom/idl-parser xpidl-parser
> +	$(call py_action,process_install_manifest,$(DIST)/idl $(DEPTH)/_build_manifests/install/dist_idl)
> +	+$(MAKE) -C $(DEPTH)/config/makefiles/xpidl xpidl

SUBMAKE ?

@@ +259,5 @@
> +	$(call BUILDSTATUS,SUBTIER_FINISH splendid external)
> +
> +.PHONY: nss
> +ifndef MOZ_NATIVE_NSS
> +nss: base nspr external

The nspr rule doesn't exist ifndef MOZ_NATIVE_NSPR (probably not a lot of people are doing --with-system-nspr --without-system-nss, but that's still something possible)

@@ +266,5 @@
> +	$(call BUILDSTATUS,SUBTIER_FINISH splendid nss)
> +endif
> +
> +.PHONY: js
> +js: base nspr

I've had problems in the past with phony rules matching an existing directory. I'd rather avoid that.

::: config/nspr/Makefile.in
@@ +9,5 @@
>  VPATH = @srcdir@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
> +EXTERNALLY_MANAGED_MAKE_FILE := 1

Why do you need that now?

::: mobile/android/app.mozbuild
@@ +9,5 @@
>  
>      include('/toolkit/toolkit.mozbuild')
>  
>  elif CONFIG['ENABLE_TESTS']:
> +    add_tier_dir('app', 'testing/mochitest')

mmmmmmmmmmmmmmm I don't know what to think about this.

::: security/build/Makefile.in
@@ +12,5 @@
>  CC_WRAPPER =
>  CXX_WRAPPER =
>  
> +EXTERNALLY_MANAGED_MAKE_FILE := 1
> +MODULE = nss

why?

Comment 3

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #2)
> ::: config/nspr/Makefile.in
> @@ +9,5 @@
> >  VPATH = @srcdir@
> >  
> >  include $(DEPTH)/config/autoconf.mk
> >  
> > +EXTERNALLY_MANAGED_MAKE_FILE := 1
> 
> Why do you need that now?

I removed the moz.build, which was worthless and not needed.

> ::: mobile/android/app.mozbuild
> @@ +9,5 @@
> >  
> >      include('/toolkit/toolkit.mozbuild')
> >  
> >  elif CONFIG['ENABLE_TESTS']:
> > +    add_tier_dir('app', 'testing/mochitest')
> 
> mmmmmmmmmmmmmmm I don't know what to think about this.

I'm assuming this was added because of silliness.

> ::: security/build/Makefile.in
> @@ +12,5 @@
> >  CC_WRAPPER =
> >  CXX_WRAPPER =
> >  
> > +EXTERNALLY_MANAGED_MAKE_FILE := 1
> > +MODULE = nss
> 
> why?

Same reason as above - deleted the moz.build file because it wasn't providing anything.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Collapse many build tiers

Incorporated feedback.

Comment 5

[Mike Hommey [:glandium]]

Attachment: PoC

Here's my take on this. I went through extreme lengths to be conservative in this patch, and make things keep building in the same order as they do now, besides the fact that export, libs and tools are done on the whole tree instead of per tier. Not tested on anything else than linux at the moment.

Comment 6

[Mike Hommey [:glandium]]

Attachment: PoC - part 2

This is the more aggressive part. It adds a new 'tier', compile, building all OBJS and HOST_OBJS, and makes export and tools entirely parallel built.
Unfortunately, while everything from export runs concurrently to the icu build, icu still keeps building for a while without other cores doing anything. I suspect it should be fine to move icu building to libs instead of export, in which case other builds could happen at the same time.

Note this completely blows up with pymake, because it actually sucks at parallel jobs.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Pseudo-derecursify the build (opt-in)

One can opt-in with "export MOZ_PSEUDO_DERECURSE=1" in a mozconfig.
I would like to land this earliest, possibly before the merge next week. It's not risky, since it's essentially NPOTB.
This sets the ground for many optimizations, like filtering out directories that aren't needed for certain tiers, massively parallel builds, and build stamping (allowing instant rebuilds, with adequate dependencies ; that will come with bug 905973)

Comment 8

[Mike Hommey [:glandium]]

Attachment: Parallelize compile and tools tiers

While previous patch was only setting ground for the future, this is a part of the future. Sadly, export can't be parallelized just yet because of bug 915622. I'll file a separate bug to parallelize export, depending on that bug.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Pseudo-derecursify the build (opt-in)

With a small fix on tools dir recursion (it was invoking make -C foo/bar tools for subdirectories of tools dirs instead of make -C foo/bar libs)

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 804145 [details] [diff] [review]
Pseudo-derecursify the build (opt-in)

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

This patch is pretty gnarly all around. It's a hard review. I worry about the cost of maintaining this code over time. That being said, a lot of it only needs to be written once - I can't imagine the feature set changing too much (at least the Python bits).

I'm supportive of the approach here. Aside from some implementation details, I believe the methodology is sound. Somewhat complicated, but sound.

recurse.mk is pretty complicated. Have you given any though to just generating a static .mk file in recursivemake.py? While the produced file will be pretty long, I /think/ it might be easier to read and to follow what's going on. It may also be faster for pymake!

Some more tests for the Python bits would be nice, notably what's written to the .mk files. But I suppose the build system itself tests that, so I'm not going to be demanding.

::: config/recurse.mk
@@ +9,5 @@
> +ifneq (.,$(DEPTH))
> +MOZ_PSEUDO_DERECURSE=
> +endif
> +
> +ifdef MOZ_PSEUDO_DERECURSE

Is it worth putting this logic in its own file so we don't incur the parsing overhead during "traversal?"

@@ +58,5 @@
> +# Carefully avoid $(eval) type of rule generation, which makes pymake slower
> +# than necessary.
> +# Get current tier and corresponding subtiers from the data in root.mk.
> +CURRENT_TIER := $(filter $(foreach tier,compile libs export tools,recurse_$(tier)),$(MAKECMDGOALS))
> +ifneq (,$(filter-out 0 1,$(words $(CURRENT_TIER))))

How do 0 and 1 get in $(CURRENT_TIER)?

@@ +67,5 @@
> +
> +# Get all directories traversed for all subtiers in the current tier, or use
> +# directly the $(*_dirs) variables available in root.mk when there is no
> +# TIERS (like for js/src).
> +CURRENT_DIRS := $(or $($(CURRENT_TIER)_dirs),$(foreach subtier,$(CURRENT_SUBTIERS),$($(CURRENT_TIER)_subtier_$(subtier))))

Yet $($(CURRENT_TIER)_subtier_js) is always defined in root.mk. That seems wrong. If it isn't used, don't define it?

@@ +101,5 @@
> +$(addprefix $(STAMPS_DIR)$(CURRENT_TIER)_,$(subst /,_,$(CURRENT_DIRS))): $(STAMPS_DIR)$(CURRENT_TIER)_%:
> +	$(call BUILDSTATUS,TIERDIR_START $(CURRENT_TIER) $(subtier_of_$*) $(or $(dir_$*),$(subst _,/,$*)))
> +	+@$(if $(recurse_$*),NO_RECURSE= )$(MAKE) -C $(DEPTH)/$(or $(dir_$*),$(subst _,/,$*)) $(if $(no_target_for_$*),,$(or $(target_for_$(@F)),$(CURRENT_TIER)))
> +	@$(STAMP_TOUCH)
> +	$(call BUILDSTATUS,TIERDIR_FINISH $(CURRENT_TIER) $(subtier_of_$*) $(or $(dir_$*),$(subst _,/,$*)))

Oy.

Let's consolidate the $(or $(dir_$*,...) bit and always use the $(dir_*) value from the .mk file. Simplicity wins by default.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +129,5 @@
> +    parallel), then static directories, dirs, tests and tools (all
> +    sequentially).
> +    """
> +    SubDirectoryTypes = ['parallel', 'static', 'dirs', 'tests', 'tools']
> +    SubDirectories = namedtuple('SubDirectories', SubDirectoryTypes)

Since you are calling the contstructor with [], [], [], ... a few times, I'd say it's almost worth it to just create a class whose __init__ does the right thing.

@@ +139,5 @@
> +        """
> +        Function signature is, in fact:
> +            def add(self, dir, parallel=[], static=[], dirs=[],
> +                               tests=[], tools=[])
> +        but it's done with **kargs to avoid repetitive code.

I don't condone this. I see where you are coming from though. And it is implemented correctly. Meh.

@@ +242,5 @@
> +            if current in self._traversal:
> +                current = None
> +            return current, parallel, dirs
> +
> +        return list(self.traverse('', external_dirs_filter))

Ugh. The code in this class is complicated!

I'd like to think that annotating this info at read time (such as my approach in bug 910453) would be much simpler than trying to deduce it later. Just look at how small the patches in that bug are! Granted they don't do as much. But I just have a hunch that approach will be cleaner and easier to maintain.

Architecturally, I'd also rather see "data derivation" like this occur in the reader or emitter layers. because backends should be as data-driven as possible. And who knows - maybe this metadata could be useful to other backends!

Can you try moving this logic to reader.py and emitter.py and pass it down to backends through the DirectoryTraversal class?

@@ +462,5 @@
> +                root_mk.add_statement('%s_subtiers := %s' % (tier, ' '.join(subtiers)))
> +            for subtier in subtiers:
> +                subtier_dirs = list(self._traversal.traverse('subtier_start_%s' % subtier, filter))
> +                # subtier_dirs[0] is 'subtier_start_%s' % subtier, and
> +                # subtier_dirs[-1] is 'subtier_finish_%s' % subtier, skip them.

At this point I attempted to locate a beer and failed.

@@ +481,5 @@
> +            # if the current directory is a tools dir, then its descendents are
> +            # too, otherwise, only subdirs.tools descendents are tools dirs.
> +            for d in subdirs.tools if current not in tools_dirs else parallel + dirs:
> +                root_mk.add_statement('target_for_tools_%s := libs' % d.replace('/', '_'))
> +                tools_dirs.add(d)

More logic I think would be better captured at read time.

@@ +484,5 @@
> +                root_mk.add_statement('target_for_tools_%s := libs' % d.replace('/', '_'))
> +                tools_dirs.add(d)
> +            return None, parallel, dirs
> +        for d in self._traversal.traverse('', get_dirs):
> +            pass

Please comment you are flushing an iterator. I wonder if there is a built-in to do this without performance sucking side-effects (list(iter) counts as performance sucking).

@@ +496,5 @@
> +                                         'root.mk')) as root, \
> +             FileAvoidWrite(os.path.join(self.environment.topobjdir,
> +                                         'root-deps.mk')) as root_deps:
> +            root_mk.dump(root, removal_guard=False)
> +            root_deps_mk.dump(root_deps, removal_guard=False)

Call self._update_from_avoid_write() instead of using FileAvoidWrite as a context manager. Since this is a common pattern, perhaps we should add a class helper that does this.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 803665 [details] [diff] [review]
Parallelize compile and tools tiers

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

Holding off on r+ until part 1 is finalized.

I was kinda surprised that I didn't see that huge of a speedup with the patches in this bug applied. My no-op gmake builds decreased from ~85s to ~70s. I was expecting sub-60s. Then again, just a few days my no-op build speeds were in the ~60s department. Did we recently regress something?

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +436,5 @@
>              for dir, deps in all_deps.items():
>                  rule = root_deps_mk.create_rule([stamp(tier, dir)])
>                  if deps is not None:
>                      rule.add_dependencies(stamp(tier, d) for d in deps if d)
> +                if tier == 'export' and dir != 'config' and not dir.startswith('subtier_'):

dir != config :(

I don't like seeing exceptions like this coded here. But, uh, I guess it's necessary. What would it take to not make it necessary? Are we going to run into other one-offs that would justify a better solution?

Comment 12

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 804145 [details] [diff] [review]
> Pseudo-derecursify the build (opt-in)
> 
> Review of attachment 804145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is pretty gnarly all around. It's a hard review. I worry about
> the cost of maintaining this code over time. That being said, a lot of it
> only needs to be written once - I can't imagine the feature set changing too
> much (at least the Python bits).

Note on the long term, where everything is in moz.build, this will probably go away.
The problem this patch addresses is allowing improvements with what we have now in moz.build. Although it doesn't do so itself.

> recurse.mk is pretty complicated. Have you given any though to just
> generating a static .mk file in recursivemake.py? While the produced file
> will be pretty long, I /think/ it might be easier to read and to follow
> what's going on. It may also be faster for pymake!

I'm almost sure it would be the contrary.
 
> ::: config/recurse.mk
> @@ +9,5 @@
> > +ifneq (.,$(DEPTH))
> > +MOZ_PSEUDO_DERECURSE=
> > +endif
> > +
> > +ifdef MOZ_PSEUDO_DERECURSE
> 
> Is it worth putting this logic in its own file so we don't incur the parsing
> overhead during "traversal?"

We can probably worry about that in a followup.

> @@ +58,5 @@
> > +# Carefully avoid $(eval) type of rule generation, which makes pymake slower
> > +# than necessary.
> > +# Get current tier and corresponding subtiers from the data in root.mk.
> > +CURRENT_TIER := $(filter $(foreach tier,compile libs export tools,recurse_$(tier)),$(MAKECMDGOALS))
> > +ifneq (,$(filter-out 0 1,$(words $(CURRENT_TIER))))
> 
> How do 0 and 1 get in $(CURRENT_TIER)?

$(words) returns the number of words in CURRENT_TIER.

> @@ +67,5 @@
> > +
> > +# Get all directories traversed for all subtiers in the current tier, or use
> > +# directly the $(*_dirs) variables available in root.mk when there is no
> > +# TIERS (like for js/src).
> > +CURRENT_DIRS := $(or $($(CURRENT_TIER)_dirs),$(foreach subtier,$(CURRENT_SUBTIERS),$($(CURRENT_TIER)_subtier_$(subtier))))
> 
> Yet $($(CURRENT_TIER)_subtier_js) is always defined in root.mk. That seems
> wrong. If it isn't used, don't define it?

Maybe the comment is not clear enough. This is for when the *root* directory doesn't define TIERS, which, when you make -C js/src is the case. $($(CURRENT_TIER)_subtier_js) *is* used.
 
> @@ +101,5 @@
> > +$(addprefix $(STAMPS_DIR)$(CURRENT_TIER)_,$(subst /,_,$(CURRENT_DIRS))): $(STAMPS_DIR)$(CURRENT_TIER)_%:
> > +	$(call BUILDSTATUS,TIERDIR_START $(CURRENT_TIER) $(subtier_of_$*) $(or $(dir_$*),$(subst _,/,$*)))
> > +	+@$(if $(recurse_$*),NO_RECURSE= )$(MAKE) -C $(DEPTH)/$(or $(dir_$*),$(subst _,/,$*)) $(if $(no_target_for_$*),,$(or $(target_for_$(@F)),$(CURRENT_TIER)))
> > +	@$(STAMP_TOUCH)
> > +	$(call BUILDSTATUS,TIERDIR_FINISH $(CURRENT_TIER) $(subtier_of_$*) $(or $(dir_$*),$(subst _,/,$*)))
> 
> Oy.
> 
> Let's consolidate the $(or $(dir_$*,...) bit and always use the $(dir_*)
> value from the .mk file. Simplicity wins by default.

Adding variables makes pymake slower. I'm avoiding those variables.

> @@ +139,5 @@
> > +        """
> > +        Function signature is, in fact:
> > +            def add(self, dir, parallel=[], static=[], dirs=[],
> > +                               tests=[], tools=[])
> > +        but it's done with **kargs to avoid repetitive code.
> 
> I don't condone this. I see where you are coming from though. And it is
> implemented correctly. Meh.
> 
> @@ +242,5 @@
> > +            if current in self._traversal:
> > +                current = None
> > +            return current, parallel, dirs
> > +
> > +        return list(self.traverse('', external_dirs_filter))
> 
> Ugh. The code in this class is complicated!
> 
> I'd like to think that annotating this info at read time (such as my
> approach in bug 910453) would be much simpler than trying to deduce it
> later. Just look at how small the patches in that bug are! Granted they
> don't do as much. But I just have a hunch that approach will be cleaner and
> easier to maintain.
> 
> Architecturally, I'd also rather see "data derivation" like this occur in
> the reader or emitter layers. because backends should be as data-driven as
> possible. And who knows - maybe this metadata could be useful to other
> backends!

I doubt it. It's very specific to how the make backend currently works, which won't be true once we have everything in a convenient form in moz.build. Heck, my long term goal is to remove that code I'm adding here.

(In reply to Gregory Szorc [:gps] from comment #11)
> Comment on attachment 803665 [details] [diff] [review]
> Parallelize compile and tools tiers
> 
> Review of attachment 803665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Holding off on r+ until part 1 is finalized.
> 
> I was kinda surprised that I didn't see that huge of a speedup with the
> patches in this bug applied. My no-op gmake builds decreased from ~85s to
> ~70s. I was expecting sub-60s.

The patches to this bug by themselves won't make no-op builds much faster. They'll make clobber build faster. Bug 915648 would improve things for export. I have a patch under work for bug 905973 that makes the compile tier take 1~2s. There are other planned changes to skip directories that will make things faster, too.

> Then again, just a few days my no-op build
> speeds were in the ~60s department. Did we recently regress something?

The addition of the compiler tier probably regressed it. I'm going to remove it for the non-pseudo-recurse case, in a followup, btw.

> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +436,5 @@
> >              for dir, deps in all_deps.items():
> >                  rule = root_deps_mk.create_rule([stamp(tier, dir)])
> >                  if deps is not None:
> >                      rule.add_dependencies(stamp(tier, d) for d in deps if d)
> > +                if tier == 'export' and dir != 'config' and not dir.startswith('subtier_'):
> 
> dir != config :(
> 
> I don't like seeing exceptions like this coded here. But, uh, I guess it's
> necessary. What would it take to not make it necessary? Are we going to run
> into other one-offs that would justify a better solution?

make export requires nsinstall, which is only built in config. That's the reason for that exception. The only way i could think of is to add a preexport tier just building config. Kind of awful.
Alternatively, those deps could be hardcoded in recurse.mk.

Comment 13

[Mike Hommey [:glandium]]

Note a way to get rid of the $(subst /,_) business is to use the paths as-is in the targets, which would make STAMP_TOUCH slightly more complicated (need make -p) but not incredibly so. This would make stamp lookup slightly slower, too, but probably not noticeably so.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Pseudo-derecursify the build (opt-in).

Also remove the compile tier added in bug 912856 when pseudo-derecursify is disabled.


This should be a bit simpler than the previous iteration.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Parallelize compile and tools tiers

Comment 16

[Mike Hommey [:glandium]]

Attachment: Pseudo-derecursify the build (opt-in).

Only small changes compared to previous patch: fix js build without the compiler tier, and partially enable BUILD STATUS with pseudo-derecurse.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 806987 [details] [diff] [review]
Pseudo-derecursify the build (opt-in).

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

This was a hard review. I wouldn't be surprised if there were subtle bugs in the recusion/traversal code. But, results matter. This code appears to do what it advertises. Bravo.

::: config/recurse.mk
@@ +58,5 @@
> +ifdef CURRENT_TIER
> +ifeq (0,$(MAKELEVEL))
> +export NO_RECURSE_MAKELEVEL=1
> +else
> +export NO_RECURSE_MAKELEVEL=$(word $(MAKELEVEL),2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20)

Oy.

@@ +69,5 @@
> +CURRENT_DIRS := $(or $($(CURRENT_TIER)_dirs),$(foreach subtier,$(CURRENT_SUBTIERS),$($(CURRENT_TIER)_subtier_$(subtier))))
> +
> +# Subtier delimiter rules
> +$(addprefix subtiers/,$(addsuffix _start/$(CURRENT_TIER),$(CURRENT_SUBTIERS))): subtiers/%_start/$(CURRENT_TIER):
> +	$(call BUILDSTATUS,SUBTIER_START $(CURRENT_TIER) $* $(if $(BUG_915535_FIXED),$($(CURRENT_TIER)_subtier_$*)))

Nice hack.

@@ +107,2 @@
>  	$(call BUILDSTATUS,TIER_START $@ $(filter-out $(if $(filter export,$@),,precompile),$(TIERS)))
> +	$(foreach tier,$(TIERS), $(if $(filter-out libs_precompile tools_precompile,$@_$(tier)), \

Now that the tiers are inverted, perhaps we should move the precompile make file out of its own tier and into base.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +132,5 @@
> +    SubDirectoryCategories = ['parallel', 'static', 'dirs', 'tests', 'tools']
> +    SubDirectoriesTuple = namedtuple('SubDirectories', SubDirectoryCategories)
> +    class SubDirectories(SubDirectoriesTuple):
> +        def __new__(self):
> +            return RecursiveMakeTraversal.SubDirectoriesTuple.__new__(self, [], [], [], [], [])

Now that you've discovered __new__, don't abuse it :)

@@ +188,5 @@
> +
> +        The default filter corresponds to a default recursive traversal.
> +        """
> +        if filter is None:
> +            filter = self.default_filter

filter = filter or self.default_filter?

@@ +193,5 @@
> +
> +        deps = {}
> +
> +        def recurse(start_node, prev_nodes=None):
> +            current, parallel, dirs = self.call_filter(start_node, filter)

Can we refer to "dirs" as "sequential" (or similar) throughout this code? Every time I read "dirs" my mind goes elsewhere. Maybe it's just me.

@@ +209,5 @@
> +            if parallel_nodes:
> +                prev_nodes = tuple(parallel_nodes)
> +            for dir in dirs:
> +                prev_nodes = recurse(dir, prev_nodes)
> +            return prev_nodes

This function made my head hurt.

@@ +421,5 @@
> +                rule = root_deps_mk.create_rule(['%s/%s' % (dir, tier)])
> +                if deps is not None:
> +                    rule.add_dependencies('%s/%s' % (d, tier) for d in deps if d)
> +            root_deps_mk.create_rule(['recurse_%s' % tier]) \
> +                        .add_dependencies('%s/%s' % (d, tier) for d in main)

This causes pymake to whine:

 0:04.21 No rule to make target 'subtiers/base/export' needed by ['<command-line>', 'subtiers/base/export']
 0:04.21 No rule to make target 'subtiers/nspr/export' needed by ['<command-line>', 'subtiers/nspr/export']
 0:04.21 No rule to make target 'subtiers/precompile/export' needed by ['<command-line>', 'subtiers/precompile/export']
 0:04.21 No rule to make target 'subtiers/nss/export' needed by ['<command-line>', 'subtiers/nss/export']
 0:04.21 No rule to make target 'subtiers/js/export' needed by ['<command-line>', 'subtiers/js/export']
 0:04.22 No rule to make target 'subtiers/platform/export' needed by ['<command-line>', 'subtiers/platform/export']
 0:04.22 No rule to make target 'subtiers/external/export' needed by ['<command-line>', 'subtiers/external/export']
 0:04.22 No rule to make target 'subtiers/app/export' needed by ['<command-line>', 'subtiers/app/export']

@@ +448,5 @@
> +                    for dir in subtier_dirs:
> +                        stamped = dir.replace('/', '_')
> +                        root_mk.add_statement('subtier_of_%s := %s' % (stamped, subtier))
> +                else:
> +                    root_mk.add_statement('%s_subtier_%s := %s' % (tier, subtier, ' '.join(subtier_dirs)))

Can you please document what all gets written out. Perhaps also throw some empty lines in there to improve readability.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 806961 [details] [diff] [review]
Parallelize compile and tools tiers

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +389,5 @@
>              return current, subdirs.parallel, \
>                  subdirs.dirs + subdirs.tests + subdirs.tools
>  
>          # compile and tools tiers use the same traversal as export, but skip
>          # precompile.

The comment now lies.

@@ +400,5 @@
> +            # kept sequential. Others are all forced parallel.
> +            if current.startswith('subtiers/') and all_subdirs and \
> +                    all_subdirs[0].startswith('subtiers/'):
> +                return current, [], all_subdirs
> +            return current, all_subdirs, []

Perhaps you should rename this to "flattened_filter" or similar.

Comment 19

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d79a54d83e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d19104a7002

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a3d79a54d83e
https://hg.mozilla.org/mozilla-central/rev/0d19104a7002