1240134 - Don't process XPIDL files during artifact build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

I recently removed the processing of WebIDL and IPDL files when --disable-compile-environment is in effect. This makes artifact builds a few seconds faster. We should do the same for XPIDL.

Unlike WebIDL and IPDL (where I just had to disable building via "ifdef COMPILE_ENVIRONMENT"), XPIDL is slightly more involved because the xpt and manifest artifacts are distributed in omni.ja (IIRC) (they aren't all compiled into the shared libraries). This violates the boundary between compiled code in chrome, but that's the world we live in. In order to disable XPIDL processing with --disable-compile-environment, we'll need to teach the artifact build mode to install the xpt and manifest files.

I reckon this will shave 4-8s off artifact build times due to skipping a lot of python build processes in the export tier.

Comment 1

[Gregory Szorc [:gps]]

I've kinda got this working, but only on Linux so far. We have to process omni.ja files to extract components/components.interfaces and components/interfaces.xpt files.

Tentative wall time improvement for a clobber build with the .jar artifacts already cached:

Before:
real    0m25.721s
user    0m52.472s
sys     0m6.324s

After:
real    0m22.852s
user    0m30.092s
sys     0m4.444s

Not the biggest wall time win (~3s). But CPU time drops significantly (~59s to ~35s) since we're no longer processing >1000 XPIDL files (in reality ~200 process invocations to produce ~200 xpt files). This will likely also translate into a more significant wall time win on Windows since we won't create >1000 files (installing .h files and creating dependency files).

In the ~23s clobber build world:

* ~3.5s configure
* ~6.1s moz.build
* ~4.0s Python package installation (surely this can be optimized)
* ~0.8s artifact archive install
* ~3.5s pre-export (mainly install manifests - tests are long pole and chmanchester is on that)
* ~0.7s export
* ~2.1s misc (feels like a lot of XPI generation)
* ~1.5s libs
* ~0.2s tools

In that ~23s build, CPU usage only averaged ~26% (over 4+4 cores).

There are a few seconds to shave here and there. <20s is achievable. I think we'll start to plateau around ~15s with in-the-pipeline improvements unless we figure out a way to saturate more cores or find some CPU abuse. Not too shabby.

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1240134 - Allow JARReader to read data already in memory; r?glandium

This will be needed to teach artifact builds to extract files from
omni.ja files whose content is loaded into memory (from a tar
archive).

Review commit: https://reviewboard.mozilla.org/r/36191/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36191/

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: TODO Bug 1240134 - Extract XPIDL components files from omni.ja

We don't need to build XPIDL files during artifact builds because
the build artifacts have the interfaces.xpt and components.manifest
files in them. Unlike executables and other files currently handled
by artifact builds, these files are in omni.ja files within the
main archive.

This commit teaches the artifact processing code to extract XPIDL
related files from omni.ja files. By installing these files from
the artifacts, we'll be able to stop processing XPIDL source files
as part of artifact builds.

This commit does introduce some overhead to extract and read omni.ja
files. However, this still takes less CPU than processing 1000+
XPIDL source files.

TODO OS X support

Review commit: https://reviewboard.mozilla.org/r/36193/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36193/

Comment 4

[Gregory Szorc [:gps]]

Attachment: MozReview Request: HACK do not install xpidl

Review commit: https://reviewboard.mozilla.org/r/36195/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36195/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8722716 [details]
MozReview Request: Bug 1240134 - Allow JARReader to read data already in memory; r?glandium

https://reviewboard.mozilla.org/r/36191/#review32859

Comment 7

[Chris Manchester (limited bugmail, email directly)]

I'm going to take a stab at landing this.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1240134 - Incorporate the interfaces.xpt from downloaded artifacts instead of building XPIDL during an artifact build.

Review commit: https://reviewboard.mozilla.org/r/69144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69144/

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8777617 [details]
Bug 1240134 - Incorporate the interfaces.xpt from downloaded artifacts instead of building XPIDL during an artifact build.

https://reviewboard.mozilla.org/r/69144/#review67534

::: browser/moz.build:41
(Diff revision 1)
> +if CONFIG['MOZ_ARTIFACT_BUILDS']:
> +    # Include "pre-built interfaces" for dist/bin/browser
> +    # This is a dummy manifest that ensures an interfaces.xpt
> +    # installed to the objdir by an artifact build is incorporated
> +    # into the build.
> +    include('/build/include-interfaces.mozbuild')

interesting that the build system wouldn't barf on you including this file twice: once here and once from build/moz.build. You should only need the latter.

::: python/mozbuild/mozbuild/artifacts.py:213
(Diff revision 1)
> +                if basename == 'omni.ja':
> +                    # Our incoming file will be in assets/, but we need to install
> +                    # to bin/, so ignore the base directory.
> +                    self._process_omni_ja(f.read(), writer)

This code shouldn't assume anything about omni.ja. It would be better to use an UnpackFinder, although it doesn't support to do its job from a Jar, but it should be straightforward to make it support the case through a JarFinder. It probably makes sense to change UnpackFinder to take an instance of a BaseFinder as argument and to dispatch there instead of dispatching to self as a FileFinder.

::: python/mozbuild/mozbuild/artifacts.py:252
(Diff revision 1)
>              with tarfile.open(filename) as reader:
>                  for f in reader:
>                      if not f.isfile():
>                          continue
>  
> +                    if mozpath.basename(f.name) == 'omni.ja':

same here, but we'd need a TarFinder.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8777617 [details]
Bug 1240134 - Incorporate the interfaces.xpt from downloaded artifacts instead of building XPIDL during an artifact build.

https://reviewboard.mozilla.org/r/69144/#review67534

> interesting that the build system wouldn't barf on you including this file twice: once here and once from build/moz.build. You should only need the latter.

I found this necessary in browser/moz.build to end up with a manifest for dist/bin/browser/components/interfaces.xpt ... maybe including another mozbuild file isn't helping this, I guess it's clearer just adding to EXTRA_COMPONENTS in build/moz.build and browser/moz.build separately.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1240134 - Re-factor UnpackFinder to contain a Finder instance it delegates to rather than inheriting from FileFinder.

Review commit: https://reviewboard.mozilla.org/r/71382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71382/

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1240134 - Implement a TarFinder to facilitate extracting files from compressed Firefox archives.

Review commit: https://reviewboard.mozilla.org/r/71384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71384/

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8777617 [details]
Bug 1240134 - Incorporate the interfaces.xpt from downloaded artifacts instead of building XPIDL during an artifact build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69144/diff/1-2/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8780746 [details]
Bug 1240134 - Re-factor UnpackFinder to contain a Finder instance it delegates to rather than inheriting from FileFinder.

https://reviewboard.mozilla.org/r/71382/#review69250

::: python/mozbuild/mozpack/packager/l10n.py:228
(Diff revision 1)
> -    app_finder = UnpackFinder(source)
> -    l10n_finder = UnpackFinder(l10n)
> +    app_finder = UnpackFinder(FileFinder(source))
> +    l10n_finder = UnpackFinder(FileFinder(l10n))

This is somehow heavyweight. Why not make UnpackFinder take a path or a BaseFinder instance, and have it create the FileFinder on its own when given a path?

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8780747 [details]
Bug 1240134 - Implement a TarFinder to facilitate extracting files from compressed Firefox archives.

https://reviewboard.mozilla.org/r/71384/#review69252

::: python/mozbuild/mozpack/files.py:524
(Diff revision 1)
> +        GeneratedFile.__init__(self, tar.extractfile(info).read())
> +        self._mode = info.mode
> +
> +    @property
> +    def mode(self):
> +        return self._mode

mmmmm it might be desirable to use the same mode normalization as File.mode (so factor it out from there).

::: python/mozbuild/mozpack/files.py:527
(Diff revision 1)
> +    @property
> +    def mode(self):
> +        return self._mode
> +
> +    def read(self):
> +        return BytesIO(self.content).read()

you can return self.content here.

::: python/mozbuild/mozpack/files.py:971
(Diff revision 1)
>          Actual implementation of JarFinder.find(), dispatching to specialized
>          member functions depending on what kind of pattern was given.
>          '''
>          return self._find_helper(pattern, self._files,
>                                   lambda x: DeflatedFile(self._files[x]))
>  

2 empty lines between classes. Here and after the class definition.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8780746 [details]
Bug 1240134 - Re-factor UnpackFinder to contain a Finder instance it delegates to rather than inheriting from FileFinder.

https://reviewboard.mozilla.org/r/71382/#review69254

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8777617 [details]
Bug 1240134 - Incorporate the interfaces.xpt from downloaded artifacts instead of building XPIDL during an artifact build.

https://reviewboard.mozilla.org/r/69144/#review69258

I think it would be better to split this patch in two, where you first convert to use UnpackFinder, and then add the interfaces.xpt.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8780746 [details]
Bug 1240134 - Re-factor UnpackFinder to contain a Finder instance it delegates to rather than inheriting from FileFinder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71382/diff/1-2/

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8780747 [details]
Bug 1240134 - Implement a TarFinder to facilitate extracting files from compressed Firefox archives.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71384/diff/1-2/

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8777617 [details]
Bug 1240134 - Incorporate the interfaces.xpt from downloaded artifacts instead of building XPIDL during an artifact build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69144/diff/2-3/

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8780746 [details]
Bug 1240134 - Re-factor UnpackFinder to contain a Finder instance it delegates to rather than inheriting from FileFinder.

Not sure whether these changes warrant re-review. Erring on the side of re-review.

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8780746 [details]
Bug 1240134 - Re-factor UnpackFinder to contain a Finder instance it delegates to rather than inheriting from FileFinder.

https://reviewboard.mozilla.org/r/71382/#review69700

::: python/mozbuild/mozpack/packager/unpack.py:42
(Diff revision 2)
>  
>      This means that for example, paths like chrome/browser/content/... match
>      files under jar:chrome/browser.jar!/content/... in case of jar chrome
>      format.
> -    '''
> -    def __init__(self, *args, **kargs):
> +
> +    The only argument to the constructor is a Finder instance or path.

"or a path"

Comment 23

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #17)
> I think it would be better to split this patch in two, where you first
> convert to use UnpackFinder, and then add the interfaces.xpt.

Let me insist on this, which was not addressed.

Comment 24

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mike Hommey [:glandium] from comment #23)
> (In reply to Mike Hommey [:glandium] from comment #17)
> > I think it would be better to split this patch in two, where you first
> > convert to use UnpackFinder, and then add the interfaces.xpt.
> 
> Let me insist on this, which was not addressed.

I'll fix this before landing.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/099a5d697513
Re-factor UnpackFinder to contain a Finder instance it delegates to rather than inheriting from FileFinder. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2c15236f5b
Implement a TarFinder to facilitate extracting files from compressed Firefox archives. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/91dfed0af1dc
Use the UnpackFinder when extracting from archives for an artifact build. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/98bc41e5c777
Incorporate the interfaces.xpt from downloaded artifacts instead of building XPIDL during an artifact build. r=glandium

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f4e45fb0783
Fixup TarFinder test on Windows by closing the TarFile after the test. r=me

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/099a5d697513
https://hg.mozilla.org/mozilla-central/rev/0c2c15236f5b
https://hg.mozilla.org/mozilla-central/rev/91dfed0af1dc
https://hg.mozilla.org/mozilla-central/rev/98bc41e5c777
https://hg.mozilla.org/mozilla-central/rev/0f4e45fb0783