1239217 - Use "faster" traversal techniques for artifact-based builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Alternate bug summary: "Integrate 'faster' traversal techniques into the recursive make backend"

On my i7-6700K on Linux, artifact based builds take ~42s with my patches from bug 1239207 and bug 1239210. ~16s is spent in `mach configure`. ~26s is spent in the build backend.

Of that ~26s:

~8.0s install manifests
~4.3s export
~3.3s misc
~9.0s libs
~0.2s tools

(that doesn't sum to 26s because the logging isn't perfect, but it's close enough)

What's crazy is if you do a `mach build faster` after a `mach configure`, it only takes ~5.7s! And, `mach build faster` does like 90% of what `mach build` is doing.

`mach build` appears to spend its time doing a lot of one-dir-at-a-time traversal during the "libs" tier (because that's how the libs tier works). This wouldn't be so bad if it weren't processing jar.mn files in many of those directories.

I reckon if we refactored jar.mn processing (along with other things we know how to build "faster"), we could probably get clobber artifact builds to the ~25s range (at least on my machine). At this point, Python package installation, moz.build reading, and install manifest processing would be long poles. But I think I'd be happy with 25s clobber build times :P

Comment 1

[Mike Hommey [:glandium]]

I'll look at hooking the faster backend to replace parts of the recursive make backend if planets are aligned right next week.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Minimalistic PoC

Greg, could you get new numbers with the same changeset you got your numbers with, with bug 1240660 and bug 1240671 applied, then with this applied?

Comment 3

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Created attachment 8709335 [details] [diff] [review]
> Minimalistic PoC
> 
> Greg, could you get new numbers with the same changeset you got your numbers
> with, with bug 1240660 and bug 1240671 applied, then with this applied?

To be clearer, the latter would also be with bug 1240660 and bug 1240671 applied.

Comment 4

[Gregory Szorc [:gps]]

For 67e23ec18a27 (which got backed out):

* 16.0s configure
*  2.5s install manifests
*  4.0s export
*  4.5s misc
*  2.4s libs
*  0.2s tools

* 16.0s configure
* 17.1s build
* 33s total

With the PoC patch in this bug applied, I don't see a major change in overall performance. But the phase times do appear to shift very slightly (could be noise though):

* 16.0s configure
*  2.7 install manifests
*  4.8s export
*  4.0s misc
*  3.2s libs
*  0.2s tools

This isn't an apples to apples comparison with comment #0 since I compared changesets a few hundreds commits apart. This includes changes to not install ~17k web platform test files, which accounts for shorter install manifest time.

Comment 5

[Mike Hommey [:glandium]]

I don't have timings at the moment, but some interesting numbers, on the number of make -C $dir $tier:

Before bug 1241022:
Ignoring 626 directories during export  (equiv. Using 359 directories during export)
Ignoring 655 directories during libs    (equiv. Using 330 directories during libs)
Using 146 directories during misc
Using 3 directories during tools
(total, excluding compile: 838)

After bug 1241022:
Using 144 directories during export
Using 198 directories during libs
Using 137 directories during misc
Using 3 directories during tools
(total, excluding compile: 482)

With my current WIP (which is delegating more than the attached PoC to FasterMake):
Using 144 directories during export
Using 114 directories during libs
Using 34 directories during misc
Using 3 directories during tools
(total, excluding compile: 295)

That can't hurt, right? :)

Comment 6

[Mike Hommey [:glandium]]

Timings on my machine, on artifacts builds, according to mach resource-usage with bug 1241416 applied (only measured once, so there is obviously noise in there): 

Before bug 1241022:
           clobber  no-op  directories
pre-export   4.0s    1.7s
export      11.2s    0.9s    317
misc         7.8s    7.3s    125
libs         8.9s    7.9s    308
tools        0.2s    0.2s      3

total       32.1s   18.0s

After bug 1241022:
           clobber  no-op  directories
pre-export   5.0s    1.7s
export      10.9s    0.6s    100
misc         7.9s    7.6s    117
libs         8.3s    7.4s    176
tools        0.2s    0.2s      3

total       32.3s   17.5s

The first observation here is that the reduction in the number of traversed directories doesn't seem to change much. That said, that's on Linux, the story might be different on Windows.

With the patches I'm about to attach:
           clobber  no-op  directories
pre-export  14.1s    2.1s
export       1.8s    0.6s    101 
misc         6.3s    4.8s     15 
libs         3.3s    1.7s     93 
tools        0.2s    0.2s      4 (yes, we grew a new directory for tools since bug 1241022 ; it's the same directory that increased export, too)

total       25.7s    9.4s

Those times (obviously) don't count configure during the clobber build or the initial artifact check.

Now, the obvious observation is that pre-export+export didn't move, but the thing is pre-export now also does things that were done in misc/libs (obviously, otherwise we wouldn't see a drop)

This doesn't get us in gps's ballpark from comment 0, but:
- the long pole of pre-export is now xpidl, and for artifacts builds, that should actually be skipped and the xpt files used from the artifacts (because of the possible mismatch with the )
- everything that we implement in FasterMake in the future will move things from export/misc/libs/tools to pre-export. I filed bug 1241743 to track these.
- there are most likely things we can do to skip more directories for export and libs.

All of those things are subjects for followup bugs.

Comment 7

[Mike Hommey [:glandium]]

Also, the long pole of misc is now bug 1240676.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Skip empty install manifests, apart a few exceptions

Install manifests are not empty in normal conditions, apart a few
exceptions where they are only used for a "magic" `rm -rf`.

However, we're going to introduce changes that will empty some of
the install manifests and make their work happen from a different
backend, in which case we don't want them to correspond to a `rm -rf`.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Add the notion of Partial and Hybrid build backends

Make the FasterMake backend a partial build backend.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Stop making the FasterMake build system refresh the backend on its own

The FasterMake build system is meant to be invoked through `mach build
faster`, which does it already, or, in the near future, as part of an
hybrid build system, which will deal with it as well. People doing
`make -C objdir/faster` won't have the backend automatically refreshed,
but that's not a supported way to use it anyways.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Make the RecursiveMake build system create backend files generically

The current rule is only for "backend.RecursiveMakeBackend", but, with
the current default of generating both the RecursiveMake and FasterMake
backends, the command creates/refreshes both backends. This is, in fact,
how the FasterMake backend is refreshed in most cases.

Moreover, with an hybrid backends, the generated file is not
"backend.RecursiveMakeBackend" anymore, so we need a more generic way to
handle this.

Furthermore, it's not necessarily desirable for all backends to have a
dependency file to handle the dependencies to refresh the backend, so
generate a plain list instead. This has the side effect of making `mach
build-backend --diff` more readable for changes to that file.

Finally, make the backend.* files created like any other backend file,
such that its diff appears in the `mach build-backend --diff` output.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Add the FasterMake+RecursiveMake hybrid backend

Comment 13

[Mike Hommey [:glandium]]

Attachment: Enable the hybrid FasterMake+RecursiveMake backend for artifact builds

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8710858 [details] [diff] [review]
Add the notion of Partial and Hybrid build backends

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

Well, this should lay some groundwork for some interesting future work. I reckon we could split handling of various sub-components of the build system into PartialBackend's now. Maybe you do that in subsequent patches...

::: python/mozbuild/mozbuild/backend/base.py
@@ +265,5 @@
> +    assert all(issubclass(b, PartialBackend) for b in backends[:-1])
> +    assert not(issubclass(backends[-1], PartialBackend))
> +    assert all(issubclass(b, BuildBackend) for b in backends)
> +
> +    class TheHybridBackend(BuildBackend):

Does this class definition need to be in this function? It should be sufficient to declare it once (with __init__ taking a backends list) and using type() to create the new type, no?

This isn't a huge deal, so feel free to ignore.

@@ +293,5 @@
> +        (b.__name__.replace('Backend', '') for b in backends[:1]),
> +        (b.__name__ for b in backends[-1:])
> +    ))
> +
> +    return type(str(name), (TheHybridBackend,), {})

I see you have discovered type() :)

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8710859 [details] [diff] [review]
Stop making the FasterMake build system refresh the backend on its own

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

I guess this patch breaks things until the subsequent refactor. Meh.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8710860 [details] [diff] [review]
Make the RecursiveMake build system create backend files generically

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

::: Makefile.in
@@ +102,5 @@
> +# the command will only run once. So:
> +#     backend%RecursiveMakeBackend backend%FasterMakeBackend:
> +#         @echo do stuff
> +#     backend: backend.RecursiveMakeBackend backend.FasterMakeBackend
> +# would only execute the command once.

It's not magic: it's GNU Make!

::: config/rules.mk
@@ +541,3 @@
>  
> +endef
> +$(foreach file,$(BUILD_BACKEND_FILES),$(eval $(call build_backend_rule,$(file))))

Should this common pattern go in an included .mk file?

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8710861 [details] [diff] [review]
Add the FasterMake+RecursiveMake hybrid backend

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

::: config/faster/rules.mk
@@ +46,5 @@
>  # Targets from the recursive make backend to be built for a default build
>  default: $(TOPOBJDIR)/config/makefiles/xpidl/xpidl
>  endif
>  
> +ifndef FASTER_RECURSIVE_MAKE

This is worthy of an inline comment.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8710862 [details] [diff] [review]
Enable the hybrid FasterMake+RecursiveMake backend for artifact builds

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

Cool.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 8710858 [details] [diff] [review]
> Add the notion of Partial and Hybrid build backends
> 
> Review of attachment 8710858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Well, this should lay some groundwork for some interesting future work. I
> reckon we could split handling of various sub-components of the build system
> into PartialBackend's now. Maybe you do that in subsequent patches...

I don't, but yes, we could separate things out and transform the existing backends in hybrid backends.

> ::: python/mozbuild/mozbuild/backend/base.py
> @@ +265,5 @@
> > +    assert all(issubclass(b, PartialBackend) for b in backends[:-1])
> > +    assert not(issubclass(backends[-1], PartialBackend))
> > +    assert all(issubclass(b, BuildBackend) for b in backends)
> > +
> > +    class TheHybridBackend(BuildBackend):
> 
> Does this class definition need to be in this function? It should be
> sufficient to declare it once (with __init__ taking a backends list) and
> using type() to create the new type, no?

See how it's used in part 5 :)

(In reply to Gregory Szorc [:gps] from comment #15)
> Comment on attachment 8710859 [details] [diff] [review]
> Stop making the FasterMake build system refresh the backend on its own
> 
> Review of attachment 8710859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this patch breaks things until the subsequent refactor. Meh.

Actually no, as vaguely explained in the commit message. We're already refreshing the backend in supported use cases.

> ::: config/rules.mk
> @@ +541,3 @@
> >  
> > +endef
> > +$(foreach file,$(BUILD_BACKEND_FILES),$(eval $(call build_backend_rule,$(file))))
> 
> Should this common pattern go in an included .mk file?

The block wasn't in an included .mk file, so I thought "meh".

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d981cf65508
https://hg.mozilla.org/integration/mozilla-inbound/rev/233be618da58
https://hg.mozilla.org/integration/mozilla-inbound/rev/848ccd8fd7da
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9996f9ea66f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d610a47138d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef27c15ebe87

Comment 21

[Gregory Szorc [:gps]]

With this on inbound and an artifact build:

             clobber     no-op
pre-export    3.07       1.16
export        4.46       0.39
misc          3.16       3.43
libs          7.19       6.16
tools         0.24       0.25
total        20.57      13.97

I wonder why our differs differ, especially the pre-export times for clobber. Were you on a stale page cache or are you running patches not on inbound?

Comment 22

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #21)
> With this on inbound and an artifact build:
> 
>              clobber     no-op
> pre-export    3.07       1.16
> export        4.46       0.39
> misc          3.16       3.43
> libs          7.19       6.16
> tools         0.24       0.25
> total        20.57      13.97
> 
> I wonder why our differs differ, especially the pre-export times for
> clobber. Were you on a stale page cache or are you running patches not on
> inbound?

I think that comes from 2 cores vs. 4 cores.

Comment 23

[Mike Hommey [:glandium]]

That said, your export time is high... and I now realize that the hybrid build system is not enabled as it's supposed to be.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee4a20245a0

Comment 25

[Gregory Szorc [:gps]]

With the M4 fixup patch:


             clobber     no-op
pre-export    8.01       1.28
export        0.82       0.35
misc          1.93       1.85
libs          2.95       1.54
tools         0.23       0.24
total        16.58       7.73

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d981cf65508
https://hg.mozilla.org/mozilla-central/rev/233be618da58
https://hg.mozilla.org/mozilla-central/rev/848ccd8fd7da
https://hg.mozilla.org/mozilla-central/rev/e9996f9ea66f
https://hg.mozilla.org/mozilla-central/rev/0d610a47138d
https://hg.mozilla.org/mozilla-central/rev/ef27c15ebe87
https://hg.mozilla.org/mozilla-central/rev/5ee4a20245a0