1483778 - When doing PGO + LTO, we should skip LTO on the profile-generate phase

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Mike Hommey [:glandium]]

There is no benefit from doing LTO on the profile-generate phase, only time loss from doing LTO for something that is not going to be actually used.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Remove MOZ_CFLAGS_NSS

It hasn't been used since bug 1295937.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Remove --enable-llvm-hacks

It calls for cargo-culting its use when using clang, when it's specific
to skipping one linker flags for sanitizers.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Don't look for llvm-symbolizer for LTO builds

Similar to bug 1478923.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Move LTO flags to python configure

Comment 5

[Mike Hommey [:glandium]]

Attachment: Skip LTO during the profile-generate phase of PGO

When both LTO and PGO are enabled, there is no point LTO'ing during the
first phase of PGO.

... or maybe there is, and time will tell. Talos on try might be indicating that it does matter, but I have enough bad experience with talos that I'd rather see how things go after landing.

If we get to the point where it turns out it's not a win, I'd like to keep some of the cleanup from the other patches, though.

Comment 6

[Mike Hommey [:glandium]]

Something worth noting: PGO+LTO on Linux without this patch currently crashes with a stack overflow (in what looks like an infinite loop) while linking/LTO'ing libxul on the profile-generate phase. It finishes properly with this patch queue applied.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #6)
> It finishes properly with this patch queue applied.

Actually correction. Clang 6 doesn't. Clang 7rc1 does. But it does without too. Clang 6 hits a different error with the patch queue applied.

Comment 8

[Mike Hommey [:glandium]]

> Talos on try might be indicating that it does matter, but I have enough bad experience with talos that I'd rather see how things go after landing.

So comparing talos on try vs talos on inbound seemed to indicate it matters, while comparing talos on try vs. talos on try without the patches seems to indicate it doesn't make a difference. So yeah... we'll see after landing.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 9001853 [details] [diff] [review]
Don't look for llvm-symbolizer for LTO builds

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

::: build/autoconf/sanitize.m4
@@ -4,5 @@
>  
>  AC_DEFUN([MOZ_CONFIG_SANITIZE], [
>  
>  dnl ========================================================
>  dnl = Link Time Optimization (LTO)

One wonders why LTO-related code is in sanitize.m4 in the first place...

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 9001852 [details] [diff] [review]
Remove --enable-llvm-hacks

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

::: old-configure.in
@@ +637,5 @@
>      if test "$GCC_USE_GNU_LD"; then
>          # Some tools like ASan use a runtime library that is only
>          # linked against executables, so we must allow undefined
>          # symbols for shared objects in some cases.
> +        if test -z "$MOZ_ASAN$MOZ_MSAN$MOZ_UBSAN$MOZ_TSAN"; then

It seems slightly better to me to have a separate variable in one place rather than this mashup whenever we need it, but meh.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 9001854 [details] [diff] [review]
Move LTO flags to python configure

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

::: build/autoconf/sanitize.m4
@@ -11,5 @@
> -    AC_DEFINE(MOZ_LTO)
> -    CFLAGS="$CFLAGS $MOZ_LTO_CFLAGS"
> -    CPPFLAGS="$CPPFLAGS $MOZ_LTO_CFLAGS"
> -    CXXFLAGS="$CXXFLAGS $MOZ_LTO_CFLAGS"
> -    LDFLAGS="$LDFLAGS $MOZ_LTO_LDFLAGS"

How much do we care that since we're not sticking these in CFLAGS et al during configure, they won't be tested during configure tests?  I guess that's OK?

::: build/moz.configure/toolchain.configure
@@ +1401,5 @@
>  
> +set_config('MOZ_LTO', lto.enabled)
> +set_define('MOZ_LTO', lto.enabled)
> +set_config('MOZ_LTO_CFLAGS', lto.cflags)
> +set_config('MOZ_LTO_LDFLAGS', lto.ldflags)

\o/

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 9001855 [details] [diff] [review]
Skip LTO during the profile-generate phase of PGO

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

My guess is that you can get better branch probabilities/block execution counts with LTO during profile-generate (e.g. LTO doing more aggressive inlining gives you more opportunities to get better data), and you're profiling code that's closer to what you'd actually generate anyway.

I'm inclined to not take this, but I am willing to hear arguments otherwise.  I assume your talos numbers are --rebuild-talos 5 with a mozilla-central base on try, not pulling numbers from mozilla-central itself?

Comment 13

[Mike Hommey [:glandium]]

--rebuild-talos 10, even. And that seems to make no difference. OTOH, it's clearly faster to build.

Comment 14

[Mike Hommey [:glandium]]

Landed the already r+ed parts in bug 1484872

Comment 15

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #13)
> --rebuild-talos 10, even. And that seems to make no difference. OTOH, it's
> clearly faster to build.

Those perf results might be skewed by sccache, actually (sccache is also the cause for the build problems I had on linux).

Comment 16

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > --rebuild-talos 10, even. And that seems to make no difference. OTOH, it's
> > clearly faster to build.
> 
> Those perf results might be skewed by sccache, actually (sccache is also the
> cause for the build problems I had on linux).

They're not, because we don't sccache anything on windows right now, because of the -Xclang arguments to clang-cl.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 9001855 [details] [diff] [review]
Skip LTO during the profile-generate phase of PGO

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

Faster build times seem nice.  I guess we'll see what talos says?

Comment 18

[Nathan Froyd [:froydnj]]

Still seems a little weird to profile code that isn't really running with the same settings as what you're going to use for the guided optimization, but maybe the differences wash out in the end.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb9a0ab83ad
Skip LTO during the profile-generate phase of PGO. r=froydnj

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3fb9a0ab83ad

Comment 21

[Ionuț Goldan [:igoldan]]

Build time improvements on Windows:

== Change summary for alert #15219 (as of Tue, 21 Aug 2018 19:06:07 GMT) ==

Improvements:

  9%  build times windows2012-64 pgo taskcluster-c5.4xlarge     4,211.70 -> 3,832.43
  7%  build times windows2012-64 pgo taskcluster-c4.4xlarge     4,969.49 -> 4,607.03
  7%  build times windows2012-32 pgo taskcluster-c4.4xlarge     4,900.12 -> 4,554.44

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=15219

If these improvements are more related to bug 1485072, just leave a comment.