1210687 - Use install manifests for jar.mn files in FasterMake backend

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Separate out jar.mn parsing in a separate class

Comment 2

[Mike Hommey [:glandium]]

Attachment: Use install manifests for jar.mn files in FasterMake backend

This gets me under 1.5s on my machine for an incremental `mach build faster`, instead of 4s, and also fixes race conditions that I had been seeing because of bug 1210703. This gets under 1s when avoiding preprocessing of all files, which requires some tracking as mentioned in bug 1210642.

On my machine, still, this bring the reticulation time to 0.2s on config.status, which seems reasonable.

Timings on automation are crazy, though:

Linux (looks good):
01:39:56     INFO -  Finished reading 2553 moz.build files in 4.87s
01:39:56     INFO -  Processed into 9463 build config descriptors in 5.00s
01:39:56     INFO -  RecursiveMake backend executed in 4.79s
01:39:56     INFO -    2553 total backend files; 2553 created; 0 updated; 0 unchanged; 0 deleted; 125 -> 961 Makefile
01:39:56     INFO -  FasterMake backend executed in 0.26s
01:39:56     INFO -    5 total backend files; 5 created; 0 updated; 0 unchanged; 0 deleted
01:39:56     INFO -  Total wall time: 17.47s; CPU time: 12.75s; Efficiency: 73%; Untracked: 2.55s

Mac (pretty pathetic):
01:43:37     INFO -  Finished reading 2528 moz.build files in 14.60s
01:43:37     INFO -  Processed into 9370 build config descriptors in 15.89s
01:43:37     INFO -  RecursiveMake backend executed in 8.56s
01:43:37     INFO -    2533 total backend files; 2533 created; 0 updated; 0 unchanged; 0 deleted; 121 -> 939 Makefile
01:43:37     INFO -  FasterMake backend executed in 1.62s
01:43:37     INFO -    5 total backend files; 5 created; 0 updated; 0 unchanged; 0 deleted
01:43:37     INFO -  Total wall time: 41.51s; CPU time: 18.13s; Efficiency: 44%; Untracked: 0.85s

Windows (abysmal):
01:45:56     INFO -  Finished reading 2573 moz.build files in 13.81s
01:45:56     INFO -  Processed into 9353 build config descriptors in 27.20s
01:45:56     INFO -  RecursiveMake backend executed in 17.11s
01:45:56     INFO -    2581 total backend files; 2581 created; 0 updated; 0 unchanged; 0 deleted; 127 -> 966 Makefile
01:45:56     INFO -  FasterMake backend executed in 2.54s
01:45:56     INFO -    5 total backend files; 5 created; 0 updated; 0 unchanged; 0 deleted
01:45:56     INFO -  Total wall time: 63.17s; CPU time: 63.16s; Efficiency: 100%; Untracked: 2.50s

While mac and windows look rather bad (> 1s for the FasterMake backend execution), that's still only 4% of the overall wall time of config.status.

Comment 3

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #2)
> Mac (pretty pathetic):

Is the Mac timing equivalent to the Linux timing on the cross-compiled Taskcluster builds?

Comment 4

[Mike Hommey [:glandium]]

Yes:
08:47:54     INFO -  Finished reading 2523 moz.build files in 3.69s
08:47:54     INFO -  Processed into 9343 build config descriptors in 3.95s
08:47:54     INFO -  RecursiveMake backend executed in 4.38s
08:47:54     INFO -    2525 total backend files; 2525 created; 0 updated; 0 unchanged; 0 deleted; 119 -> 938 Makefile
08:47:54     INFO -  FasterMake backend executed in 0.23s
08:47:54     INFO -    5 total backend files; 5 created; 0 updated; 0 unchanged; 0 deleted
08:47:54     INFO -  Total wall time: 12.60s; CPU time: 11.54s; Efficiency: 92%; Untracked: 0.36s

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8668857 [details] [diff] [review]
Separate out jar.mn parsing in a separate class

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

I'm so happy jar parsing is now separated from action processing!

This is a large patch. I wish it were split up a bit more. But, we have tests for jar manifest processing and a lot of automation that relies on it. Important thing is correct results and I'm confident we'll have that given coverage of this code.

::: python/mozbuild/mozbuild/jar.py
@@ +101,5 @@
> +    def __init__(self):
> +        self._current_jar = None
> +        self._jars = []
> +
> +    def write(self, line):

It initially felt strange to see write() on a parser class. But I see this is necessary because of the preprocessor usage below. Worth a comment.

@@ +105,5 @@
> +    def write(self, line):
> +        if self._current_jar is None:
> +            m = self.jarline.match(line)
> +            if not m:
> +                raise RuntimeError(line)

Not a huge fan of RuntimeError here, but it is backwards compatible. So meh.

@@ +111,5 @@
> +                self._current_jar = JarInfo(m.group('jarfile'))
> +                self._jars.append(self._current_jar)
> +            return
> +
> +        if self.ignore.match(line):

This feels like it should be moved to first.

@@ +119,5 @@
> +        if m:
> +            if self._current_jar.chrome_manifests or self._current_jar.entries:
> +                self._current_jar = JarInfo(self._current_jar.name)
> +                self._jars.append(self._current_jar)
> +            self._current_jar.relativesrcdir = m.group('relativesrcdir')

I'd *really* appreciate if you took the time to insert some inline comments, as jar manifests aren't exactly an intuitive data format/structure.

@@ -247,2 @@
>          pp.do_include(infile)
> -        lines = PushbackIter(pp.out.getvalue().splitlines())

I /think/ this is the only use of PushbackIter in the tree. Please remove it if so.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8668859 [details] [diff] [review]
Use install manifests for jar.mn files in FasterMake backend

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

::: python/mozbuild/mozbuild/backend/fastermake.py
@@ +32,5 @@
> +    def _add_entry(self, dest, entry):
> +        # Because of bug 1210703, we can't let the default behavior of
> +        # InstallManifest._add_entry, which is to error out.
> +        # To match the current behavior of the recursive make libs tier, we
> +        # keep the last one given, but still warn about it.

I ran into this issue when trying to not stage C++ tests as part of the test archive refactor. That's why we still stage C++ tests for archiving :/

Two use cases in a short period of time makes me think we should consider an argument on InstallManifest.__init__ to relax its duplication behavior. Although arguably in both cases it is due to poor underlying behavior. Although, one could imagine a use case for "stacked directories" which is kinda sorta what's in play in both cases. I'm not going to block review on this, however.

@@ +34,5 @@
> +        # InstallManifest._add_entry, which is to error out.
> +        # To match the current behavior of the recursive make libs tier, we
> +        # keep the last one given, but still warn about it.
> +        if dest in self._dests:
> +            print('Warning: Item already in manifest: %s' % dest,

Would adding a manifest path to the message help with debugging?

@@ +195,5 @@
> +        # this would require all the magic surrounding l10n and addons in
> +        # the recursive make backend to die, which is not going to happen
> +        # any time soon enough.
> +        # Notably missing:
> +        # - DEFINES from config/config.mk

At some point we need to talk about moving config.mk's logic to moz.build, as this opens up a whole new world of possibilities...

Comment 7

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 8668859 [details] [diff] [review]
> Use install manifests for jar.mn files in FasterMake backend
> 
> Review of attachment 8668859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/backend/fastermake.py
> @@ +32,5 @@
> > +    def _add_entry(self, dest, entry):
> > +        # Because of bug 1210703, we can't let the default behavior of
> > +        # InstallManifest._add_entry, which is to error out.
> > +        # To match the current behavior of the recursive make libs tier, we
> > +        # keep the last one given, but still warn about it.
> 
> I ran into this issue when trying to not stage C++ tests as part of the test
> archive refactor. That's why we still stage C++ tests for archiving :/

I'm not sure what specifically you're talking about, but for C++ tests, it's
arguably worse, since they are built in parallel tiers, so there is no determinism.

> @@ +34,5 @@
> > +        # InstallManifest._add_entry, which is to error out.
> > +        # To match the current behavior of the recursive make libs tier, we
> > +        # keep the last one given, but still warn about it.
> > +        if dest in self._dests:
> > +            print('Warning: Item already in manifest: %s' % dest,
> 
> Would adding a manifest path to the message help with debugging?

It would, but it would also require more work to actually track where that came
from. I'd rather just fix bug 1210703 and remove this exception.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #5)
> I /think/ this is the only use of PushbackIter in the tree. Please remove it
> if so.

I'll take this as a r+ on a separate patch removing this function.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b09f6fe508f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31e57477fc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9702af7611ca

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b09f6fe508f3
https://hg.mozilla.org/mozilla-central/rev/e31e57477fc7
https://hg.mozilla.org/mozilla-central/rev/9702af7611ca
https://hg.mozilla.org/mozilla-central/rev/da83c31bf39c