850380 - Build XPIDLs in parallel, nonrecursively

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

It would be rad to compile IDLs in parallel so we can build the tree faster.

This will likely involve ripping out the existing $(IDLSRCS) logic in rules.mk and replacing it with either:

a) an automatically generated .mk file containing all the rules and logic for compiling IDLs.

b) an automatically generated .mk file defining lists of things supplemented with a new, static .mk file that turns those variables into rules.

Other considerations:

* Eventual interaction with Tup
* How to handle leaf-directory builds
* We could potentially leverage packager code to manage dist/idl so we don't have to blow it out at the top of builds
* We could compile all IDLs in the same directory
* We might be able to get rid of -I from XPIDL_FLAGS

Leaf-directory builds (make -C foo) are interesting. Presumably if we build all IDLs in parallel they are evaluated as a single make target. That make target would presumably be evaluated by the top-level Makefile at the beginning of a build. What happens when someone types |make -C toolkit|. Do we evaluate that rule? Do we ignore IDLs? Do leaf builds know of their subset of IDLs that should be rebuilt? How do we traverse the tree without redundantly evaluating the monolithic IDLs target?

We should probably consider all these before code is written.

Comment 1

[Mike Hommey [:glandium]]

I'd go for a python script handling the entire thing, which would require bug 819048. That would avoid a big overhead of running a new interpreter for each idl file.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> I'd go for a python script handling the entire thing, which would require
> bug 819048. That would avoid a big overhead of running a new interpreter for
> each idl file.

(in fact i was planning to do it eventually, but you can beat me to it if you want)

Comment 3

[Gregory Szorc [:gps]]

Until we have bug 819048, we could always call pymake and implement compiling as native pymake commands.

Comment 4

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #0)
> It would be rad to compile IDLs in parallel so we can build the tree faster.
> 
> This will likely involve ripping out the existing $(IDLSRCS) logic in
> rules.mk and replacing it with either:
> 
> a) an automatically generated .mk file containing all the rules and logic
> for compiling IDLs.
> 
> b) an automatically generated .mk file defining lists of things supplemented
> with a new, static .mk file that turns those variables into rules.
> 
> Other considerations:
> 
> * Eventual interaction with Tup
> * How to handle leaf-directory builds
> * We could potentially leverage packager code to manage dist/idl so we don't
> have to blow it out at the top of builds
> * We could compile all IDLs in the same directory
> * We might be able to get rid of -I from XPIDL_FLAGS

I don't have a suggestion for how this should be handled in make, but tup currently compiles all IDLs from the dist/idl directory using xpcom/idl-parser/header.py. XPIDL_FLAGS is actually ignored since all .idl files are created in dist/idl before running header.py. Given the concerns in this bug, the tup build:

* Compiles .idl -> .h in parallel
* Does not remove dist/idl at the top of builds
* Compiles all .idl files in the same directory
* Does not use XPIDL_FLAGS; as far as tup is concerned, this could be removed

I believe any improvements to make will not impact the tup build, given the following:

1) The definition of XPIDLSRCS (now moz.build/XPIDL_SOURCES) is kept in the per-directory build fragments

2) header.py is unchanged, or at least still allows the current model of compiling a single .idl file per invocation.

Comment 5

[Gregory Szorc [:gps]]

Over to joey. We should probably etherpad up a plan of attack first...

Comment 6

[Gregory Szorc [:gps]]

I'm taking this one.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Build IDLs in parallel, v1

I've had this patch sitting around for over a week and figured I'd post it so people can see the direction. It still needs some tests, changes to the auto-generated .mk file, and support for partial tree builds.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Build IDLs in parallel, v2

Here is the latest version. It's still a little rough around the edges and could use some tests.

Essentially the build backend collects IDL files during moz.build traversal. At the end of traversal it writes out objdir/_idl/idls.mk. The content of the file is derived from /build/idl/idls.mk.in.

The targets are mostly statically defined in that file, making figuring out how IDLs are created relatively straightforward. The exception is the targets defining how .idl files are installed from the source dir. We explicitly list the source filenames because VPATH on dozens of directories is silly.

The auto-generated idls.mk is inserted into tier traversal by introducing the new directory /build/idl/. The Makefile in this directory has a special rule to invoke idls.mk.

idls.mk is actually invoked twice! On the first invocation, it installs all the .idl files from the source directory into a unified directory in the object directory. On the second, it processes all the -> .h and -> .xpt rules. idls.mk has a comment on why it is this way. The tl;dr is that the .h and .xpt rules depend on an unknown subset of .idl files to work. We need to ensure *every* .idl is present before we attempt these rules. If we introduced a normal make dependency, every .h/.xpt would depend on every .idl and changing a single .idl would invalidate the world. Not cool.

I could have added order-only prerequisites, but that would break pymake.

I could have added -I include path searching to IDL operations to include every source file. But, I reckon that would result in excessive stat() calls, would make commands excessively long, and would just be ugly. I think staging all the .idl files in one invocation is a simple and elegant solution.

I might be missing a feature where you can do this in make. However, phony targets are either always or never up to date and this would undermine our dependencies.

I'm still working out Windows support. And I'd like to clean up the code a bit more as well as consider inevitable Tup integration (e.g. we should move the calculation of the commands to create .h/.xpt into the emitter.py umbrella so both make and Tup can use the same logic for determining how to do stuff). I'd also like to clean up the dependencies for how the Python tools are generated. Currently that logic is fragmented and appears to be partially redundant.

Comment 9

[Gregory Szorc [:gps]]

Oh, I think I can also now get rid of XPIDL_MODULE. AFAICT it was only being used to formulate the filename of the intermediate .xpt file.

Also, I wonder if there are other optimizations we can take now. e.g. perhaps we can eliminate the intermediate .xpt files altogether and just derive the final idls.xpt file...

Comment 10

[Mike Hommey [:glandium]]

Yes, linking intermediate xpts is a waste of time. The packager will link everything together. BUT, all the xpts need to be added to the manifest under $(FINAL_TARGET) (currently only the intermediate xpts are), AND package manifests need to be updated. The latter is the most problematic part.

Comment 11

[(not currently active) Ted Mielczarek]

Can we not list them by wildcard in the package manifest? That would seem like the most sensible solution.

(And if we have XPT files that we aren't shipping that currently wind up in dist/bin/components, then we should fix that.)

Comment 12

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> (And if we have XPT files that we aren't shipping that currently wind up in
> dist/bin/components, then we should fix that.)

we do. but fixing that is opening a whole can of worms that's probably not worth blocking upon to go forward with this bug. So i'd advise keeping the intermediate xpts and fix all these issues in a followup.

Comment 13

[Gregory Szorc [:gps]]

Attachment: Build IDLs in parallel, v3

I'm slowly iterating there.

The big change with this version is the .xpt and .h are produced via a single command invocation. This required creating a new .py file with the logic to write out both at once.

I see significant perf wins by doing it this way: it almost halved execution time (it shaved over 90s off CPU time)! This tells me that the bottleneck is in parsing. I suspect we're in a similar boat as bug 861587 where the overhead for parsing every file in a separate context is significant. We may eventually prune this down to a single command invocation to perform the heavy lifting. But, follow-up methinks.

Question for khuey:

* Now that I effectively duplicated main() from header.py and typelib.py, do these routines still need to exist or can I remove them? AFAICT nobody is really using them outside the build system...

Comment 14

[Gregory Szorc [:gps]]

It appears we define a bunch of .xpt in the packager manifests:

https://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#139
https://mxr.mozilla.org/mozilla-central/source/b2g/installer/package-manifest.in#129
https://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#85

This patch keeps getting bigger and bigger...

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 743970 [details] [diff] [review]
Build IDLs in parallel, v3

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

::: build/idl/idls.mk.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +{header}

It would probably be less confusing if the file had another extension than .in, or if it was using @var@ constructs like other .in files.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #14)
> It appears we define a bunch of .xpt in the packager manifests:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/installer/package-
> manifest.in#139
> https://mxr.mozilla.org/mozilla-central/source/b2g/installer/package-
> manifest.in#129
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/
> package-manifest.in#85
> 
> This patch keeps getting bigger and bigger...

That's why I've been advocating to keep XPIDL_MODULE for now.

Comment 17

[Gregory Szorc [:gps]]

Attachment: Build IDLs in parallel, v4

And I finally got Windows/pymake to build properly! Turns out pymake doesn't like something about wildcard rules like:

  %.done: %.idl

However, if I do something like:

  $(done_files): %.done: %.idl

pymake works just fine. I wish pymake worked like GNU make. But I'll take what I can get!

I still need to clean up the code a bit, add tests, add the non-recursive targets, etc. I'm looking for feedback mostly on the make bits. Do you approve of how I add a new tier for IDL processing? Do you think producing a monolithic idls.mk is a good one? Do you agree that the $(MAKE) calls are sane? Do you see any obvious errors with the dependencies or optimizations we could easily implement?

After this lands, I think next steps include:

* Figuring out the state of all the XPIDL .py scripts in the tree and consolidating code, especially the build system glue.
* Using $? to process multiple IDLs in a single process. I /think/ executing everything in a single process will be faster than starting up N = cores processes because of new process overhead on Windows and the overhead of redundantly parsing IDLs.

Comment 18

[Gregory Szorc [:gps]]

Attachment: Example idls.mk

Here is the idls.mk my OS X build produces.

Comment 19

[Gregory Szorc [:gps]]

Attachment: Example dependency file

Here is an example dependency file.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 750875 [details] [diff] [review]
Build IDLs in parallel, v4

Actually, this is pretty close to a review. I'll just request strict scrutiny. I fully expect an r-.

Comment 21

[Gregory Szorc [:gps]]

Attachment: Build IDLs in parallel, v5

OK, I'm much happier with this version and think it is close to landing.

I added a mozpack entity - DirectoryPruner - for comparing a FileRegistry against a directory to remove excess files not in the registry. This is similar to logic in FileCopier, but since we don't have file content, I couldn't use FileCopier.

I removed the silly idl.py file from previous patch versions and significantly reduced the complexity of the code therein.

At the end of moz.build traversal, the backend constructs a DirectoryPruner instance from the set of output files that backend knows will be produced. We then purge old files from the objdir if the corresponding .idl has been deleted! I figure this will be the proving ground of this technique. We can eventually extend this to directories in dist/ so we don't have to completely blow away directories at the top of the build.

I've tested the partial tree non-recursive targets and they seem to work.

There are basic unit tests. They are no worse than our existing tests over XPIDL generation: none.

I'm not sure what's left for this except for review and refinement.

Try at https://tbpl.mozilla.org/?tree=Try&rev=c60d068b856b.

Comment 22

[Gregory Szorc [:gps]]

Attachment: Build XPIDLs with moz.build-derived non-recursive make file

Fixed xpcshell failure due to Makefile.in target removal.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 751401 [details] [diff] [review]
Build XPIDLs with moz.build-derived non-recursive make file

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

::: build/idl/Makefile.in
@@ +17,5 @@
> +# the staging area before we begin processing. And, the only way to accomplish
> +# that is by having each .h and .xpt target depend on every .idl or have a
> +# phony target. Phony targets are never up to date, so that would invalidate
> +# no-op builds. And, having each .h and .xpt depend on every .idl would create
> +# more dependencies than actually exist, slowing down incremental builds.

We have a list of all the idls and directories they live in. Why not just use the directories list as include list when treating the idls, and set the dependencies with $(topsrcdir)-relative paths? That would avoid having to stage them, which, as you're doing it, also leaves idls around on incremental builds when they are removed in the source tree.

Comment 24

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #23)
> We have a list of all the idls and directories they live in. Why not just
> use the directories list as include list when treating the idls, and set the
> dependencies with $(topsrcdir)-relative paths? That would avoid having to
> stage them, which, as you're doing it, also leaves idls around on
> incremental builds when they are removed in the source tree.

The files don't remain around in the stage directory because of the new directory pruner.

The reason I stage the files is (possibly premature) optimization. I'm making the IDL parser work less by not having to stat a few dozen directories. Will this result in any perf win, I'm not sure. I do know Windows has a horrible stat cache by default and this just might make things faster. Will that cancel out the overhead from copying all the files into the stage directory, probably not.

I'll remove the staging directory.

Comment 25

[Gregory Szorc [:gps]]

I take that back. As it is currently implemented, we have a unified static pattern rule for everything involving .idl files (except the initial stage). If we were to leave .idl files in srcdir, we'd need a separate rule for each source directory. Doable, sure. But, this seems more complicated. It might even be slower because make will be doing more work. I kinda like the cleanliness and simplicity of the current solution.

Comment 26

[Gregory Szorc [:gps]]

Attachment: Build XPIDLs with moz.build-derived non-recursive make file, v7

The big change is I moved the make logic from idls.mk into Makefile.in.
Makefile now includes the autogenerated xpidls.mk file. Instead of
calling out to custom targets in the new non-recursive make file, I just
overloaded export and libs. A nice side effect of this is that
processing the xpidl tools occurs in parallel with .idl staging during
the export subtier. While I was here, I also moved things to build under
build/xpidl instead of topobjdir/_idl. No sense introducing another root
directory not tied to anything!

Try is looking green:
https://tbpl.mozilla.org/?tree=Try&rev=945263f6075c

Comment 28

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

Comment on attachment 752450 [details] [diff] [review]
Build XPIDLs with moz.build-derived non-recursive make file, v7

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

::: js/src/config/rules.mk
@@ +23,5 @@
>    TOOL_DIRS \
>    XPIDL_MODULE \
>    $(NULL)
>  
> +_DEPRECATED_VARIABLES := \

I tend to prefer "obsolete" for things that don't work anymore, and "deprecated" for things that still work but are bad practice.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +97,5 @@
>  
>      def close(self):
> +        if self.idls:
> +            basenames = sorted(idl.basename for idl in self.idls)
> +            modules = sorted(set(idl.module for idl in self.idls))

Put this all in a separate function, perhaps?

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +154,5 @@
>      'MODULE': (unicode, unicode, "",
>          """Module name.
>  
>          Historically, this variable was used to describe where to install header
> +        files, but that feature is now handled by EXPORTS_NAMEAPACES. MODULE

name-what?

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 752450 [details] [diff] [review]
Build XPIDLs with moz.build-derived non-recursive make file, v7

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

I wonder if this patch breaks --with-libxul-sdk builds.

::: build/xpidl/Makefile.in
@@ +39,5 @@
> +parser_dir := $(topsrcdir)/xpcom/idl-parser
> +cache_dir := $(DEPTH)/xpcom/idl-parser
> +
> +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(parser_dir) -I$(cache_dir) \
> +    $(parser_dir)/generate-h-and-xpt.py $(idl_stage_dir) $(cache_dir) $(idl_gen_dir)

I'd prefer cache_dir to be behind a flag, like it is with header.py and friends. The others would probably be better as flags too.

@@ +52,5 @@
> +xpt_basenames := $(addsuffix .xpt,$(roots))
> +done_basenames := $(addsuffix .done,$(roots))
> +xpt_link_basenames := $(addsuffix .xpt,$(extra_xpt_link_roots))
> +
> +staged_idls := $(foreach idl,$(idl_basenames),$(addprefix $(idl_stage_dir)/,$(idl)))

$(addprefix $(idl_stage_dir)/,$(idl_basenames))
etc.
(that is, no need for foreach)

@@ +66,5 @@
> +export:: $(staged_idls)
> +
> +libs:: $(done_files) $(linked_xpt_files)
> +
> +ifndef NO_DIST_INSTALL

NO_DIST_INSTALL is a per-directory Makefile.in thing. I don't see it have any use here.

@@ +67,5 @@
> +
> +libs:: $(done_files) $(linked_xpt_files)
> +
> +ifndef NO_DIST_INSTALL
> +dist_idls := $(foreach idl,$(idl_basenames),$(addprefix $(DIST)/idl/,$(idl)))

no foreach.

@@ +70,5 @@
> +ifndef NO_DIST_INSTALL
> +dist_idls := $(foreach idl,$(idl_basenames),$(addprefix $(DIST)/idl/,$(idl)))
> +dist_headers := $(foreach header,$(header_basenames),$(addprefix $(DIST)/include/,$(header)))
> +
> +libs:: $(dist_idls) $(dist_headers)

headers are required on export, not libs. I know that since you're using a different tier, it's not a direct problem, but i, for one, occasionally do toplevel make export, and that would be broken.

@@ +73,5 @@
> +
> +libs:: $(dist_idls) $(dist_headers)
> +
> +$(dist_idls): $(DIST)/idl/%.idl: $(idl_stage_dir)/%.idl
> +	$(INSTALL) $< $(DIST)/idl/

If we're exporting all idls to DIST/idl anyways, why not use that as the "stage" directory?

@@ +76,5 @@
> +$(dist_idls): $(DIST)/idl/%.idl: $(idl_stage_dir)/%.idl
> +	$(INSTALL) $< $(DIST)/idl/
> +
> +$(dist_headers): $(DIST)/include/%.h: $(idl_gen_dir)/%.done
> +	$(INSTALL) $(basename $<).h $(DIST)/include/

Likewise, if we're putting all the headers in DIST/include, why not generate them directly instead of using an intermediate gen/ directory?

@@ +83,5 @@
> +
> +# We generate the .h and intermediate .xpt files with a single command
> +# invocation. This saves some parsing redundancy. Until bug 869120 is
> +# fixed, we can't use a multiple target rule because pymake will choke.
> +# So, we use a dummy file to hold state.

https://www.gnu.org/software/automake/manual/html_node/Multiple-Outputs.html says that the following should work, without the .done files:
     data.c: data.foo
             foo data.foo
     data.h: data.c

@@ +93,5 @@
> +$(header_files): $(idl_gen_dir)/%.h: $(idl_gen_dir)/%.done
> +$(xpt_files): $(idl_gen_dir)/%.xpt: $(idl_gen_dir)/%.done
> +
> +ifeq (libs,$(MAKECMDGOALS))
> +depends_files := $(wildcard $(idl_gen_dir)/*.pp)

It would be better to only include the ones corresponding to the targets we have (so, based on $(roots))

::: build/xpidl/xpidls.mk.template
@@ +5,5 @@
> +
> +{header}
> +
> +roots := {roots}
> +extra_xpt_link_roots := {extra_xpt_link_roots}

Why not use the preprocessor here instead, like for every other config.status source?

::: config/makefiles/nonrecursive.mk
@@ +39,5 @@
>  #
>  # Will get turned into the following:
>  #
>  # exports::
> +#     cd $$(DEPTH) $(MAKE) -f /path/to/exports.mk $(DIST)/include/foo.h $(DIST)/include/bar.h

missing &&, and extra $ for $(DEPTH) ; note this is equivalent to $(MAKE) -C $(DEPTH) -f /path/to/exports.mk, so you might as well replace with that.

@@ +51,5 @@
>  $(1)::
> +ifneq (,$(3))
> +	cd $$(DEPTH) && $$(MAKE) -f $(3) $(2)
> +else
> +	$$(MAKE) -C $(4) $(2)

which means this can simply become something like:
$$(MAKE) -C $(or $(4),$$(DEPTH)) $(addprefix -f ,$(3)) $(2)

::: config/rules.mk
@@ +1588,5 @@
>  #   dependency directory in the object directory, where we really need
>  #   it.
>  
>  ifneq (,$(filter-out all chrome default export realchrome tools clean clobber clobber_all distclean realclean,$(MAKECMDGOALS)))
> +MDDEPEND_FILES		:= $(strip $(wildcard $(foreach file,$(OBJS) $(PROGOBJS) $(HOST_OBJS) $(HOST_PROGOBJS) $(TARGETS),$(MDDEPDIR)/$(notdir $(file)).pp) $(addprefix $(MDDEPDIR)/,$(EXTRA_MDDEPEND_FILES))))

You've been bitrotted.

::: modules/libjar/objs.mk
@@ -16,5 @@
> -MODULES_LIBJAR_LXPIDLSRCS = \
> -		nsIZipReader.idl \
> -		nsIJARChannel.idl \
> -		nsIJARURI.idl \
> -		nsIJARProtocolHandler.idl \

Filed bug 875244 for this dumb objs.mk file.

::: python/mozbuild/mozbuild/backend/common.py
@@ +42,5 @@
> +        if not allow_existing and entry['basename'] in self.idls:
> +            raise Exception('IDL already registered: %' % entry['basename'])
> +
> +        self.idls[entry['basename']] = entry
> +        self.modules.setdefault(entry['module'], set()).add(entry['root'])

self.modules.setdefault(entry['module'], set(entry['root']))

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +104,5 @@
> +
> +            libs_targets = ['gen/%s.done' % root for root in roots]
> +
> +            libs_targets.extend('gen/%s.xpt' % root for root in roots)
> +            libs_targets.extend('xpt/%s.xpt' % root for root in modules)

Considering you're doing xpt and headers in one pass, and considering headers need to be done in export, there's no point making xpt targets libs targets. Just put everything in export. The headers are not listed, by the way.

@@ +349,5 @@
> +        stage_dir = path.join(base_dir, 'stage')
> +        gen_dir = path.join(base_dir, 'gen')
> +        xpt_dir = path.join(base_dir, 'xpt')
> +
> +        pruner = DirectoryPruner()

DirectoryPruner is ambiguous. It feels like you need to add the files to prune, while it's the opposite.

@@ +387,5 @@
> +        install_rules = []
> +        roots = []
> +
> +        for k in sorted(manager.idls):
> +            idl = manager.idls[k]

for k, idl in sorted(manager.idls.iteritems())

@@ +404,5 @@
> +            depends = ['$(idl_gen_dir)/%s.xpt' % b for b in modules[k]]
> +            xpt_link_rules.extend([
> +                '$(idl_xpt_dir)/%s.xpt: %s' % (k, ' '.join(depends)),
> +                '\t@echo "$(notdir $@)"',
> +                '\t$(xptlink) $@ $^',

For future improvement, it would be nicer if we had rule generators, like "gen_rule(target='$(idl_xpt_dir)/%s.xpt' % k, deps=depends, commands=['@echo "$(notdir $@)"', '$(xptlink) $@ $^'])" that would do the right formatting (especially for commands, with \t)

::: python/mozbuild/mozpack/copier.py
@@ +195,5 @@
> +            if not files and not dirs:
> +                os.removedirs(root)
> +                dir_count += 1
> +
> +        return file_count, dir_count

Instead of almost duplicating the FileCopier.copy code, you could just do something like this (modulo the name that, as i said is ambiguous):

class DirectoryPruner(FileCopier):
  class FakeFile(object):
    def copy(self, dest, skip_if_older=True):
      return True
      
  def add(self, path):
    FileCopier.add(path, FakeFile())

  def prune(self, dest):
    FileCopier.copy(dest)

You'll need to adjust to get back file count, but that could be worth adding to FileCopier anyways (with a count of file copied (not skipped))

Comment 30

[Gregory Szorc [:gps]]

Assigning arbitrary buildfaster priority.

Comment 31

[Gregory Szorc [:gps]]

(In reply to Mike Hommey (high latency until June 25) [:glandium] from comment #29)
> ::: build/xpidl/Makefile.in
> @@ +39,5 @@
> > +parser_dir := $(topsrcdir)/xpcom/idl-parser
> > +cache_dir := $(DEPTH)/xpcom/idl-parser
> > +
> > +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(parser_dir) -I$(cache_dir) \
> > +    $(parser_dir)/generate-h-and-xpt.py $(idl_stage_dir) $(cache_dir) $(idl_gen_dir)
> 
> I'd prefer cache_dir to be behind a flag, like it is with header.py and
> friends. The others would probably be better as flags too.

I don't disagree with you. That being said, the make logic to generate the cache is a bit convoluted at the moment. It lives in a separate make file and definitely needs cleaned up. I was thinking it would be best to save this for a follow-up bug.

I can bloat the scope of this patch if you want!

> @@ +66,5 @@
> > +export:: $(staged_idls)
> > +
> > +libs:: $(done_files) $(linked_xpt_files)
> > +
> > +ifndef NO_DIST_INSTALL
> 
> NO_DIST_INSTALL is a per-directory Makefile.in thing. I don't see it have
> any use here.

Good catch. I *casually* audited existing NO_DIST_INSTALL instances and I don't believe there are any in a directory with XPIDL files. If we need to add this back in, I reckon we can do that. But let's keep it out until proven otherwise.

> 
> @@ +70,5 @@
> > +ifndef NO_DIST_INSTALL
> > +dist_idls := $(foreach idl,$(idl_basenames),$(addprefix $(DIST)/idl/,$(idl)))
> > +dist_headers := $(foreach header,$(header_basenames),$(addprefix $(DIST)/include/,$(header)))
> > +
> > +libs:: $(dist_idls) $(dist_headers)
> 
> headers are required on export, not libs. I know that since you're using a
> different tier, it's not a direct problem, but i, for one, occasionally do
> toplevel make export, and that would be broken.

Oh, I didn't realize we had tier traversal hooked up to |make export|! I agree I should change this back to export.

> @@ +73,5 @@
> > +
> > +libs:: $(dist_idls) $(dist_headers)
> > +
> > +$(dist_idls): $(DIST)/idl/%.idl: $(idl_stage_dir)/%.idl
> > +	$(INSTALL) $< $(DIST)/idl/
> 
> If we're exporting all idls to DIST/idl anyways, why not use that as the
> "stage" directory?

Sure.

> @@ +76,5 @@
> > +$(dist_idls): $(DIST)/idl/%.idl: $(idl_stage_dir)/%.idl
> > +	$(INSTALL) $< $(DIST)/idl/
> > +
> > +$(dist_headers): $(DIST)/include/%.h: $(idl_gen_dir)/%.done
> > +	$(INSTALL) $(basename $<).h $(DIST)/include/
> 
> Likewise, if we're putting all the headers in DIST/include, why not generate
> them directly instead of using an intermediate gen/ directory?

Headers are slightly different. Since we blow away dist/include at the beginning of builds, we want the header "restore" to be a simple symlink instead of a full regen. Once we stop blowing away dist/include, we can generate direct to dist/include.

> 
> @@ +83,5 @@
> > +
> > +# We generate the .h and intermediate .xpt files with a single command
> > +# invocation. This saves some parsing redundancy. Until bug 869120 is
> > +# fixed, we can't use a multiple target rule because pymake will choke.
> > +# So, we use a dummy file to hold state.
> 
> https://www.gnu.org/software/automake/manual/html_node/Multiple-Outputs.html
> says that the following should work, without the .done files:
>      data.c: data.foo
>              foo data.foo
>      data.h: data.c

I initially tried this! However, I also ran into issues with pymake. I can't recall exactly what they are. I thought I had filed a bug.

I tried a few different versions of the rule syntax and using a .done was the only way I could get the dependencies to play well. It's possible I missed the magic combination to get pymake working with GNU's preferred syntax.

> @@ +93,5 @@
> > +$(header_files): $(idl_gen_dir)/%.h: $(idl_gen_dir)/%.done
> > +$(xpt_files): $(idl_gen_dir)/%.xpt: $(idl_gen_dir)/%.done
> > +
> > +ifeq (libs,$(MAKECMDGOALS))
> > +depends_files := $(wildcard $(idl_gen_dir)/*.pp)
> 
> It would be better to only include the ones corresponding to the targets we
> have (so, based on $(roots))

Done.

> ::: build/xpidl/xpidls.mk.template
> @@ +5,5 @@
> > +
> > +{header}
> > +
> > +roots := {roots}
> > +extra_xpt_link_roots := {extra_xpt_link_roots}
> 
> Why not use the preprocessor here instead, like for every other
> config.status source?

The variables I'm substituting are coming from RecursiveMakeBackend.py, not config.status. So, I would need to use Preprocessor.py by hand. At that point, I think it's just as simple to use Python's built-in .format(). That being said, it is nice to have consistency everywhere. So, I'll consider switching.

> ::: python/mozbuild/mozbuild/backend/common.py
> @@ +42,5 @@
> > +        if not allow_existing and entry['basename'] in self.idls:
> > +            raise Exception('IDL already registered: %' % entry['basename'])
> > +
> > +        self.idls[entry['basename']] = entry
> > +        self.modules.setdefault(entry['module'], set()).add(entry['root'])
> 
> self.modules.setdefault(entry['module'], set(entry['root']))

Not quite. This will fail to update the existing entry.

> @@ +349,5 @@
> > +        stage_dir = path.join(base_dir, 'stage')
> > +        gen_dir = path.join(base_dir, 'gen')
> > +        xpt_dir = path.join(base_dir, 'xpt')
> > +
> > +        pruner = DirectoryPruner()
> 
> DirectoryPruner is ambiguous. It feels like you need to add the files to
> prune, while it's the opposite.

Naming is hard :/

> @@ +387,5 @@
> > +        install_rules = []
> > +        roots = []
> > +
> > +        for k in sorted(manager.idls):
> > +            idl = manager.idls[k]
> 
> for k, idl in sorted(manager.idls.iteritems())
> 
> @@ +404,5 @@
> > +            depends = ['$(idl_gen_dir)/%s.xpt' % b for b in modules[k]]
> > +            xpt_link_rules.extend([
> > +                '$(idl_xpt_dir)/%s.xpt: %s' % (k, ' '.join(depends)),
> > +                '\t@echo "$(notdir $@)"',
> > +                '\t$(xptlink) $@ $^',
> 
> For future improvement, it would be nicer if we had rule generators, like
> "gen_rule(target='$(idl_xpt_dir)/%s.xpt' % k, deps=depends, commands=['@echo
> "$(notdir $@)"', '$(xptlink) $@ $^'])" that would do the right formatting
> (especially for commands, with \t)

I totally agree! A while back I had some patches that added a statement manipulation and construction API to pymake. You could programmatically build up make statements then serialize them to make syntax. It might be worth reviving those patches someday...

> ::: python/mozbuild/mozpack/copier.py
> @@ +195,5 @@
> > +            if not files and not dirs:
> > +                os.removedirs(root)
> > +                dir_count += 1
> > +
> > +        return file_count, dir_count
> 
> Instead of almost duplicating the FileCopier.copy code, you could just do
> something like this (modulo the name that, as i said is ambiguous):
> 
> class DirectoryPruner(FileCopier):
>   class FakeFile(object):
>     def copy(self, dest, skip_if_older=True):
>       return True
>       
>   def add(self, path):
>     FileCopier.add(path, FakeFile())
> 
>   def prune(self, dest):
>     FileCopier.copy(dest)
> 
> You'll need to adjust to get back file count, but that could be worth adding
> to FileCopier anyways (with a count of file copied (not skipped))

I hate DRY, so I'll definitely consider this! I may even separate these changes into their own patch to make the review easier and to unblock others making use of the code. I know I'd love to start hooking up other build directories to a "DirectoryPruner" so we don't have to do so much rm -rf on build start.

Comment 32

[Gregory Szorc [:gps]]

Attachment: Build XPIDLs with moz.build-derived non-recursive make file

I've rebased on top of bug 884587 and incorporated a lot of feedback from the
last review. However, I haven't incorporated all of the feedback yet, so
not throwing this up for review.

We now copy the .idl directly into dist/idl and we
generate the header directly into dist/include.

I still don't have generation happening during export, as requested. I'm
not sure how I'll be able to do this without invoking the make file
twice. See comment #4. Essentially we need for .idl copying to occur
before any codegen. However, we still want proper dependencies on
subsequent runs, otherwise we rebuild the world. This would
traditionally be implemented with order-only prerequisites. However,
those aren't supported in pymake. In an earlier version of the patch, I
had a make file recipe invoking another make file. This was very hacky.
There might be a way to accomplish what we need with phony targets
and/or touching .done files. However, nothing is coming to mind.

joey, mshal?

Comment 33

[Gregory Szorc [:gps]]

Attachment: Build XPIDLs with moz.build-derived non-recursive make file

Fixed tests.

Comment 34

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #32)
> I still don't have generation happening during export, as requested. I'm
> not sure how I'll be able to do this without invoking the make file
> twice. See comment #4. Essentially we need for .idl copying to occur
> before any codegen. However, we still want proper dependencies on
> subsequent runs, otherwise we rebuild the world. This would
> traditionally be implemented with order-only prerequisites. However,
> those aren't supported in pymake. In an earlier version of the patch, I
> had a make file recipe invoking another make file. This was very hacky.
> There might be a way to accomplish what we need with phony targets
> and/or touching .done files. However, nothing is coming to mind.
> 
> joey, mshal?

Maybe I'm a bit rusty, but I don't see a way to do it without order-only prerequisites. Both PHONY targets and .done files would force all .idls to be re-compiled, and I don't believe there's a way to just serialize the install & compile steps without serializing the whole Makefile (eg: with .NOTPARALLEL)

How hard is it to get order-only prereqs into pymake? Is that the only blocker for it?

Comment 35

[Benjamin Smedberg]

I may be confused, but why are we using make rules to do this at all? Or if we actually do need to do this via makefiles, why not just do all the copying to dist/idl first and then run the entire rest of it as a single pass?

I object to order-only prerequisites in general, because they are very difficult to get right without implementing make in sequential action, which is exactly opposite of how pymake operates.

Comment 36

[Gregory Szorc [:gps]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> I may be confused, but why are we using make rules to do this at all? Or if
> we actually do need to do this via makefiles, why not just do all the
> copying to dist/idl first and then run the entire rest of it as a single
> pass?

We're using make rules to get easy parallelism. We could use mozpack's FileCopier to install files. I had proposed this in bug 884587. While I think this is inevitable, I was hoping to avoid it for this patch.

I was thinking we could write out a "file install" manifest (like we're doing in bug 884587). Then, we add a target that "applies" this manifest e.g.

.install.done: install.manifest
    $(PYTHON) $(topsrcdir)/config/install_from_manifest.py $<
    @$(TOUCH) $@

$(done_files): .install.done
    ...

However, touching .install.done would invalidate $(done_files) and force rebuild. Hmm :/

The driving force creating all this confusion is backwards compatibility. People want to run leaf Makefiles manually or want things like |make export| to work as they've always done.

Comment 37

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #36)
> The driving force creating all this confusion is backwards compatibility.
> People want to run leaf Makefiles manually or want things like |make export|
> to work as they've always done.

If you want make export -C dir to do kind of like what it does now, but better, then what you need to do should be different: have make export keep track, for each directory, the files it copied. Next time make export is called, compare that list to the new list of things to copy, refresh the files that changed, install the new files, and remove the ones that were removed from moz.build.

Comment 39

[Gregory Szorc [:gps]]

Attachment: Part 1: Don't rely on VPATH for XPIDL_SOURCES

I can't believe I'm dignifying this patch with a comment.

Comment 40

[Gregory Szorc [:gps]]

Attachment: Part 2: Derecursify XPIDL processing as part of precompile tier

Latest/greatest version of the patch. Only minor changes since last
version. Probably need to refine a bit more. Uploading mainly to solicit
feedback before final review so there aren't any surprises.

Comment 41

[Gregory Szorc [:gps]]

Attachment: Part 3: Remove XPIDL_FLAGS from moz.build

PIDL_FLAGS aren't needed any more (they were only used for defining
extra includes paths). Since we now stage all .idl files into a common
directory before processing XPIDL files, we no longer need to supplement
include paths and we no longer need XPIDL_FLAGS.

This patch could be merged into part 2. But I figured it best to isolate
the code from the build config changes.

Review should be a rubber stamp, methinks.

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 786140 [details] [diff] [review]
Part 2: Derecursify XPIDL processing as part of precompile tier

Looking for feedback so I know what to work on before review.

I'd *really* like to get this landed in the next week or two.

glandium for the code and such. jcranmer for the c-c angle.

Comment 43

[Mike Hommey [:glandium]]

Comment on attachment 786140 [details] [diff] [review]
Part 2: Derecursify XPIDL processing as part of precompile tier

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

::: config/makefiles/xpidl/Makefile.in
@@ +15,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +# Now we define our custom logic for building xpidl files.
> +# Building XPIDLs effectively consists of two steps:
> +#   1) Staging all .idl files to a common directory.

Could you remind me Why this step is needed? (besides the fact that that's necessary for the SDK)

@@ +40,5 @@
> +# TODO we should use py_action, but that would require extra directories to be
> +# in the virtualenv.
> +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(parser_dir) -I$(cache_dir) \
> +    $(topsrcdir)/python/mozbuild/mozbuild/action/xpidl-process.py \
> +        $(dist_idl_dir) $(cache_dir) $(dist_include_dir) $(idl_gen_dir) $(idl_gen_dir)

As mentioned in comment 29, cache_dir would be better behind a flag.

@@ +42,5 @@
> +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(parser_dir) -I$(cache_dir) \
> +    $(topsrcdir)/python/mozbuild/mozbuild/action/xpidl-process.py \
> +        $(dist_idl_dir) $(cache_dir) $(dist_include_dir) $(idl_gen_dir) $(idl_gen_dir)
> +
> +xptlink := $(PYTHON) $(topsrcdir)/xpcom/typelib/xpt/tools/xpt.py link

Any reason not do idl->h, idl->xpt and xptlink in one pass? (that is, not store the intermediate .xpt files and output the linked xpt directly)?

@@ +71,5 @@
> +# So, we use a dummy file to hold state.
> +$(done_files): $(idl_gen_dir)/%.done: $(dist_idl_dir)/%.idl
> +	@echo "$(notdir $<)"
> +	$(idlprocess) $(notdir $<)
> +	$(TOUCH) $@

As mentioned in comment 29, you don't need the intermediate dummy file.

::: config/makefiles/xpidl/xpidls.mk.template
@@ +5,5 @@
> +
> +{header}
> +
> +roots := {roots}
> +extra_xpt_link_roots := {extra_xpt_link_roots}

cf. comment 29.

Comment 44

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #43)
> ::: config/makefiles/xpidl/Makefile.in
> @@ +15,5 @@
> > +include $(topsrcdir)/config/rules.mk
> > +
> > +# Now we define our custom logic for building xpidl files.
> > +# Building XPIDLs effectively consists of two steps:
> > +#   1) Staging all .idl files to a common directory.
> 
> Could you remind me Why this step is needed? (besides the fact that that's
> necessary for the SDK)

It isn't strictly required for processing. However, having all the files in a common directory will dramatically reduce file lookups because the XPIDL parser won't have to search multiple directories for includes. Since we install into a common directory for SDK packaging, why not reuse this effort?

> @@ +40,5 @@
> > +# TODO we should use py_action, but that would require extra directories to be
> > +# in the virtualenv.
> > +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(parser_dir) -I$(cache_dir) \
> > +    $(topsrcdir)/python/mozbuild/mozbuild/action/xpidl-process.py \
> > +        $(dist_idl_dir) $(cache_dir) $(dist_include_dir) $(idl_gen_dir) $(idl_gen_dir)
> 
> As mentioned in comment 29, cache_dir would be better behind a flag.

Fixed in next patch.

> @@ +42,5 @@
> > +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(parser_dir) -I$(cache_dir) \
> > +    $(topsrcdir)/python/mozbuild/mozbuild/action/xpidl-process.py \
> > +        $(dist_idl_dir) $(cache_dir) $(dist_include_dir) $(idl_gen_dir) $(idl_gen_dir)
> > +
> > +xptlink := $(PYTHON) $(topsrcdir)/xpcom/typelib/xpt/tools/xpt.py link
> 
> Any reason not do idl->h, idl->xpt and xptlink in one pass? (that is, not
> store the intermediate .xpt files and output the linked xpt directly)?

We can certainly do this. This might cut down on run time too since I think the idl parser will reuse old parse results when processing multiple files. I'll look into it.

> @@ +71,5 @@
> > +# So, we use a dummy file to hold state.
> > +$(done_files): $(idl_gen_dir)/%.done: $(dist_idl_dir)/%.idl
> > +	@echo "$(notdir $<)"
> > +	$(idlprocess) $(notdir $<)
> > +	$(TOUCH) $@
> 
> As mentioned in comment 29, you don't need the intermediate dummy file.

I'm pretty sure I wrote this, but I was initially working around a pymake bug. I'll test again on pymake and do it properly if pymake is less buggy.

Comment 45

[Gregory Szorc [:gps]]

Attachment: Part 2: Derecursify XPIDL processing as part of precompile tier

This is a somewhat large refactor. The make file rules have been
completely overhauled. Instead of invoking a process for every input
.idl file and following up with an xpt link for all grouped .xpt files,
we now invoke a process for each set of eventually linked-together .idl
files. This outputs a .h file for each input .idl, a .pp file, and a
linked .xpt file.

The make rules for the new world are much simpler. We simply create
dependencies on the linked .xpt files and all is well. We don't even
bother creating dependencies for the .h files because, well, I don't
believe we need to.

Reducing the invocation count of the XPIDL parser reduces total CPU time
by about 80s on my machine! Wall time decreases from ~28 to ~8s. No-op
builds are actually no-op builds and complete in under 0.25s. If you
ignore the idl install manifest, no-op builds take less than 0.05s! The
0.25s is slower than no-op build times for WebIDL and IPDL. Dependencies
appear to work.

I'm not asking for review because this patch doesn't work. Applications
will crash on startup with this patch. I've seen this problem before
during development of this patch. It has to do with the xpt files being
wrong. This isn't caught until we run a binary that uses
XPCOM and we surprisingly don't do this during a regular build. Past
experience has taught me this is likely something silly. The fix should
likely be only a line or two.

Comment 46

[Gregory Szorc [:gps]]

Attachment: Part 2: Derecursify XPIDL processing as part of precompile tier

I fixed the app start issue. I accidentally deleted XPT_NAME from the
backend.mk file so the xpt's weren't getting installed to the components
directory. Derp.

I also added dependencies on the Python modules back in.

I'm pretty happy with this version of the patch and think it's close to
landing. Things that could be improved:

* Test coverage (although this patch is likely no worse than other build
  system patches)
* Final .xpt install rule is still in rules.mk. This would require
  putting dist/bin under manifest control. IMO that's followup
  territory.

https://tbpl.mozilla.org/?tree=Try&rev=5e24a2dbb5b2

Comment 47

[Gregory Szorc [:gps]]

This patch appears to reduce "no-op" top-level builds by ~3s on my MBP (GNU make). This is not as good as I was hoping for. The bright side is that this patch removes a lot of work from recursive make traversal and gets us one step closer to eliminating the export tier, which *will* shave many seconds off the no-op build time.

There are significant wins with this patch for clobber builds. We eliminated many process calls by processing multiple XPIDL source files in one invocation, avoiding reparsing by doing so. And, since we process all XPIDL files in parallel as part of a single make file, we can fan out to as many cores as the system has. Furthermore, we schedule these tasks in parallel with WebIDL and IPDL processing, which were previously only utilizing 2/N cores during the precompile tier. The precompile tier is now mostly CPU saturated for most of its life. Although, WebIDL and IPDL appear to take longer to process than XPIDL. Hopefully once we land more things in precompile WebIDL and XPIDL won't be the long poles.

I observed the following times for clobber builds with a fully populated ccache on my MBP:

Before
real	3m56.889s
user	9m47.097s
sys	3m52.517s

real	4m35.684s
user	9m2.189s
sys	3m35.549s

real	4m35.604s
user	9m31.679s
sys	3m45.835s

After
real	3m27.289s
user	6m40.643s
sys	2m45.189s

real	4m7.846s
user	6m24.141s
sys	2m36.581s

real	3m58.291s
user	6m23.682s
sys	2m34.831s

The variance is a little higher than I'd like (I didn't aggressively kill other processes on my machine like I usually do when collecting timings). But, there appears to be a ~30s wall and 3-4 minute CPU time reduction. Not too shabby, especially when you consider the percentage decreases!

Comment 48

[Mike Hommey [:glandium]]

Comment on attachment 786746 [details] [diff] [review]
Part 2: Derecursify XPIDL processing as part of precompile tier

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

Mostly nits. I'd still like to take a look at next iteration, but this is looking good.

::: config/makefiles/precompile/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +CONFIGURE_SUBST_FILES += [
> +    '../xpidl/Makefile',
> +]

Why do you need this?

::: config/makefiles/xpidl/Makefile.in
@@ +13,5 @@
> +STANDALONE_MAKEFILE := 1
> +
> +include $(topsrcdir)/config/rules.mk
> +
> +# Now we define our custom logic for building xpidl files.

This comment line can probably go away.

@@ +19,5 @@
> +#   1) Staging all .idl files to a common directory.
> +#   2) Doing everything with the .idl files.
> +#
> +# Each .idl file is converted into a .h and .xpt file which share the
> +# root stem filename of the input file. Some .idl files belong to the

The intermediate xpt files are not built anymore.

@@ +24,5 @@
> +# same "module." We link their corresponding .xpt files together and put
> +# them in a common directory. Finally, we install files into the
> +# distribution directory (if configured).
> +
> +# This is where we put the original output of xpidl parsing.

which is not what is stored there anymore ;)

@@ +25,5 @@
> +# them in a common directory. Finally, we install files into the
> +# distribution directory (if configured).
> +
> +# This is where we put the original output of xpidl parsing.
> +idl_gen_dir := gen

why not .deps, now that you only put .pp files there?

@@ +37,5 @@
> +
> +# TODO we should use py_action, but that would require extra directories to be
> +# in the virtualenv.
> +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(IDL_PARSER_DIR) -I$(IDL_PARSER_CACHE_DIR) \
> +    $(process_py) $(dist_idl_dir) $(IDL_PARSER_CACHE_DIR) $(dist_include_dir) \

As per comment 29, the cache dir would be better behind a flag like with other scripts.

@@ +46,5 @@
> +
> +linked_xpt_files := $(addprefix $(idl_xpt_dir)/,$(addsuffix .xpt,$(xpidl_modules)))
> +depends_files := $(foreach root,$(xpidl_modules),$(idl_gen_dir)/$(root).pp)
> +
> +xpidl:: $(call mkdir_deps,gen xpt)

$(idl_gen_dir) $(idl_xpt_dir) instead of gen xpt. Although this should probably go as a dependency of linked_xpt_files instead of xpidl.

@@ +56,5 @@
> +ifdef .PYMAKE
> +-includedeps $(depends_files)
> +else
> +-include $(depends_files)
> +endif

We should probably have a $(call includedeps) at some point.

::: config/makefiles/xpidl/xpidls.mk.template
@@ +4,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +{header}
> +
> +xpidl_modules := {xpidl_modules}

Same as comment 29. There's no reason not to use the same preprocessing (Preprocessor.py) as other build files.

::: python/mozbuild/mozbuild/action/xpidl-process.py
@@ +32,5 @@
> +
> +        if path.endswith('.pyc'):
> +            path = path[0:-1]
> +
> +        deps.add(path)

I saw something similar for backend.RecursiveMakefileBackend.built.pp, we may want to factor at some point.

@@ +51,5 @@
> +        xpts[basename] = xpt
> +
> +        deps |= set(dep.replace('\\', '/') for dep in idl.deps)
> +
> +        with open(header_path, 'wb') as fh:

FileAvoidWrite ?

@@ +55,5 @@
> +        with open(header_path, 'wb') as fh:
> +            print_header(idl, fh, path)
> +
> +    xpt_path = os.path.join(xpt_dir, '%s.xpt' % module)
> +    xpt_link(xpts.values()).write(xpt_path)

writefd to a FileAvoidWrite ?

@@ +58,5 @@
> +    xpt_path = os.path.join(xpt_dir, '%s.xpt' % module)
> +    xpt_link(xpts.values()).write(xpt_path)
> +
> +    deps_path = os.path.join(gen_dir, '%s.pp' % module)
> +    with open(deps_path, 'w') as fh:

FileAvoidWrite?

@@ +69,5 @@
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument('inputdir',
> +        help='Directory in which to find source .idl files.')
> +    parser.add_argument('cachedir',
> +        help='Directory in which to write cached lexer data, etc')

Directory in which to find or write cached lexer data

::: python/mozbuild/mozbuild/backend/common.py
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from __future__ import unicode_literals
> +
> +import mozpack.path as path

mozpath?

@@ +31,5 @@
> +
> +        entry = {
> +            'source': source,
> +            'module': module,
> +            'basename': basename,

basename doesn't have the same meaning here and in xpidl-process.py, which is unfortunate.

@@ +39,5 @@
> +        if not allow_existing and entry['basename'] in self.idls:
> +            raise Exception('IDL already registered: %' % entry['basename'])
> +
> +        self.idls[entry['basename']] = entry
> +        self.modules.setdefault(entry['module'], set()).add(entry['root'])

self.modules.setdefault(entry['module'], set(entry['root']))

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +14,5 @@
>  from mozpack.manifests import (
>      InstallManifest,
>      PurgeManifest,
>  )
> +import mozpack.path as path

mozpath?

@@ +399,5 @@
> +        base_dir = path.join(manager.topobjdir, 'config', 'makefiles', 'xpidl')
> +
> +        build_files = self._purge_manifests['xpidl']
> +
> +        for p in ('xpidls.mk', 'Makefile', 'backend.mk', 'gen/mkdir.done',

.mkdir.done (with a leading dot).

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +96,5 @@
> +            if sandbox['NO_DIST_INSTALL']:
> +                raise SandboxValidationError('NO_DIST_INSTALL no longer '
> +                    'supported when defined with XPIDL_SOURCES. Please file '
> +                    'a Core :: Build Config bug instead of working around '
> +                    'this limitation.')

I wonder if this couldn't just be a warning. As in, leave NO_DIST_INSTALL working for other stuff in that directory, but ignored for xpidl. OTOH, there's probably no reason for a directory containing idls to have NO_DIST_INSTALL anyways.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +294,5 @@
>      'MODULE': (unicode, unicode, "",
>          """Module name.
>  
>          Historically, this variable was used to describe where to install header
> +        files, but that feature is now handled by EXPORTS_NAMEAPACES. MODULE

typo: EXPORT_NAMESPACES.

::: python/mozbuild/mozbuild/test/backend/common.py
@@ +100,5 @@
>          return ConfigEnvironment(srcdir, objdir, **config)
>  
>      def _emit(self, name, env=None):
>          if not env:
> +            env = env or self._get_environment(name)

the "env or" seems useless, when in a "not env" branch. Seems to me you wanted to remove the if as well.

::: xpcom/idl-parser/Makefile.in
@@ +60,5 @@
>    xpidlyacc.pyc \
>    xpidl_debug \
>    $(NULL)
> +
> +GARBAGE += $(addprefix $(IDL_PARSER_CACHE_DIR)/,$(garbage_files))

Not sure why you're adding IDL_PARSER_CACHE_DIR everywhere in this file, but it doesn't hurt, besides readability.

Comment 49

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #48)
> Comment on attachment 786746 [details] [diff] [review]
> Part 2: Derecursify XPIDL processing as part of precompile tier
> 
> Review of attachment 786746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly nits. I'd still like to take a look at next iteration, but this is
> looking good.

Excellent review! I've incorporated all of your feedback except for a few items listed below and will post a patch soon.

> ::: config/makefiles/precompile/moz.build
> @@ +5,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +CONFIGURE_SUBST_FILES += [
> > +    '../xpidl/Makefile',
> > +]
> 
> Why do you need this?

It was because the directory isn't referenced by a *DIRS variable, so the file wasn't being generated. I changed this in the patch so this isn't required.

> @@ +37,5 @@
> > +
> > +# TODO we should use py_action, but that would require extra directories to be
> > +# in the virtualenv.
> > +idlprocess := $(PYTHON_PATH) $(PLY_INCLUDE) -I$(IDL_PARSER_DIR) -I$(IDL_PARSER_CACHE_DIR) \
> > +    $(process_py) $(dist_idl_dir) $(IDL_PARSER_CACHE_DIR) $(dist_include_dir) \
> 
> As per comment 29, the cache dir would be better behind a flag like with
> other scripts.

Ahh, I misinterpreted your request to want a variable defining the cache dir (in config.mk - which I added) and not an actual flag on the command. I've fixed this.

> @@ +56,5 @@
> > +ifdef .PYMAKE
> > +-includedeps $(depends_files)
> > +else
> > +-include $(depends_files)
> > +endif
> 
> We should probably have a $(call includedeps) at some point.

Indeed. Bug 904740 will tackle as a followup.
 
> ::: config/makefiles/xpidl/xpidls.mk.template
> @@ +4,5 @@
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +{header}
> > +
> > +xpidl_modules := {xpidl_modules}
> 
> Same as comment 29. There's no reason not to use the same preprocessing
> (Preprocessor.py) as other build files.

I finally fixed this in the next patch. I even merged Makefile.in and xpidls.mk into a single file.

> ::: python/mozbuild/mozbuild/action/xpidl-process.py
> @@ +32,5 @@
> > +
> > +        if path.endswith('.pyc'):
> > +            path = path[0:-1]
> > +
> > +        deps.add(path)
> 
> I saw something similar for backend.RecursiveMakefileBackend.built.pp, we
> may want to factor at some point.

Bug 904743 filed.

> @@ +39,5 @@
> > +        if not allow_existing and entry['basename'] in self.idls:
> > +            raise Exception('IDL already registered: %' % entry['basename'])
> > +
> > +        self.idls[entry['basename']] = entry
> > +        self.modules.setdefault(entry['module'], set()).add(entry['root'])
> 
> self.modules.setdefault(entry['module'], set(entry['root']))

This won't work. setdefault will set the value to the 2nd argument iff the key isn't in the dict. Since we need to add an entry to the set, we always have to call .add().

> ::: xpcom/idl-parser/Makefile.in
> @@ +60,5 @@
> >    xpidlyacc.pyc \
> >    xpidl_debug \
> >    $(NULL)
> > +
> > +GARBAGE += $(addprefix $(IDL_PARSER_CACHE_DIR)/,$(garbage_files))
> 
> Not sure why you're adding IDL_PARSER_CACHE_DIR everywhere in this file, but
> it doesn't hurt, besides readability.

I config-ified the cache directory. The old Makefile assumed the current directory was the cache directory. While it didn't change, it could, so the rules needed updating. I concede our handling around the cache is suboptimal. I think it falls in followup bug territory.

Comment 50

[Gregory Szorc [:gps]]

Attachment: Part 2: Derecursify XPIDL processing as part of precompile tier

I'm feeling pretty good about this version.

Comment 51

[Mike Hommey [:glandium]]

Comment on attachment 789737 [details] [diff] [review]
Part 2: Derecursify XPIDL processing as part of precompile tier

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

::: config/makefiles/xpidl/Makefile.in
@@ +48,5 @@
> +        $(dist_include_dir) $(idl_xpt_dir) $(idl_deps_dir)
> +
> +xpidl_modules := @xpidl_modules@
> +
> +@rules@

xpidl_rules?

::: python/mozbuild/mozbuild/backend/configenvironment.py
@@ +167,5 @@
>          '''
>          assert(isinstance(file, basestring))
>          return os.path.normpath(os.path.join(self.topsrcdir, "%s.in" % relpath(file, self.topobjdir)))
>  
> +    def create_config_file(self, path, extra=None):

**extra?

Comment 52

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #51)
> ::: python/mozbuild/mozbuild/backend/configenvironment.py
> @@ +167,5 @@
> >          '''
> >          assert(isinstance(file, basestring))
> >          return os.path.normpath(os.path.join(self.topsrcdir, "%s.in" % relpath(file, self.topobjdir)))
> >  
> > +    def create_config_file(self, path, extra=None):
> 
> **extra?

I'm not a fan of ** for this kind of API. I tend to reserve ** for wrapping other functions and when the API is well-defined. I can imagine this API changing over time and I have a feeling ** may bite us in the future (e.g. if we remove a named argument, it will get sucked up into the variables dict).

Comment 53

[Gregory Szorc [:gps]]

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

Comment 54

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa1f28c6edf

I pushed a Windows bustage fix (I hope). We got a "bad magic" error on Windows. I encountered this during development of the patch. It is due to writing the xpt file in text mode as opposed to binary mode. This regression in the patch occurred when I switched xpt writing to use FileAvoidWrite as part of the last round of review changes. I simply reverted that part. Will file a follow-up to give FileAvoidWrite a file open mode.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Backed out for PGO bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b334e6d0a02b

https://tbpl.mozilla.org/php/getParsedLog.php?id=26547520&tree=Mozilla-Inbound

Comment 56

[Gregory Szorc [:gps]]

Ugh. So the underlying problem here is that PGO builds run |make clean| which blows out all of dist since dist is in /Makefile's GARBAGE_DIRS. The new make rules don't contain rules to account for missing $(DIST)/include/%.h files. So when PGO blows away dist/include, we don't reinstall the .h files.

There are a few ways to handle this. I'll try poking some people on IRC.

Comment 57

[Gregory Szorc [:gps]]

Attachment: Ensure headers are installed

OK. I came up with 3 ways to fix the bustage. I think this one is best
for now.

With this patch, we generate headers into the build directory then
install them into $(DIST)/include. This requires hooking up some extra
make rules, obviously. As the comment in the Makefile says, this kinda
sucks. But, the alternative is regenerating the .h files when
$(DIST)/include is blown away by |make clean| by |make
maybe_clobber_profilebuild| as part of the PGO build on Linux.

My first attempt at solving this used the multiple output file recipe at
https://www.gnu.org/software/automake/manual/html_node/Multiple-Outputs.html
(test -f + rm + make and all) to detect missing .h files in
$(DIST)/include to force reprocessing. A downside with this is we would
regenerate everything if the .h files were missing. That's wasted CPU.
Another problem was if you deleted the .h files and ran with -j > N,
make would invoke the same process multiple times in parallel! Fixing
this requires an obnoxious recipe (see the aforementioned link). And, it
still wouldn't prevent the overhead of reprocessing .idl files. So, I
abandoned this idea.

The 3rd idea (which I think is the preferred solution) is to have |make
clean| use manifests to purge $(DIST). However, I wasn't comfortable
changing that at this juncture. It's likely we'll want a separate set of
manifests to manage purging during the PGO build, etc. I figured this
was a rabbit hole not worth going down. I documented the silliness in
the Makefile so we can hopefully address this in the future.

https://tbpl.mozilla.org/?tree=Try&rev=9323d0cd66f5

Comment 58

[Gregory Szorc [:gps]]

Merged last patch and existing and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/13f3b8949f63

Comment 59

[Gregory Szorc [:gps]]

After I pushed this, I noticed an issue with the new xpidl Makefile not getting regenerated properly. The reason was the naive config.status --file mode was being used to update it instead of the recursivemake.py logic. glandium granted r+ over IRC to a simple patch to force regen to go through the proper mechanism.

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

Comment 60

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13f3b8949f63
https://hg.mozilla.org/mozilla-central/rev/0d9bf482b0d0

Comment 61

[[e-mail]]

Comment on attachment 790523 [details] [diff] [review]
Ensure headers are installed

>+$(dist_headers): $(dist_include_dir)/%.h: $(idl_headers_dir)/%.h
>+	$(INSTALL) $< $(dist_include_dir)
That seems to install each header separately; I was wondering whether it would be faster to $(INSTALL) $(xpidl_headers) $(dist_include_dir) in one go?

Comment 62

[Gregory Szorc [:gps]]

(In reply to [e-mail] from comment #61)
> Comment on attachment 790523 [details] [diff] [review]
> Ensure headers are installed
> 
> >+$(dist_headers): $(dist_include_dir)/%.h: $(idl_headers_dir)/%.h
> >+	$(INSTALL) $< $(dist_include_dir)
> That seems to install each header separately; I was wondering whether it
> would be faster to $(INSTALL) $(xpidl_headers) $(dist_include_dir) in one go?

Probably. The temporary staging BS will all go away eventually once we have comprehensive knowledge of dist/include and can use manifests for manage the I/O.