Closed Bug 1483778 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Remove MOZ_CFLAGS_NSS (obsolete) — Splinter Review
It hasn't been used since bug 1295937.
Assignee: nobody → mh+mozilla
Attached patch Remove --enable-llvm-hacks (obsolete) — Splinter Review
It calls for cargo-culting its use when using clang, when it's specific
to skipping one linker flags for sanitizers.
Similar to bug 1478923.
Attachment #9001853 - Flags: review?(core-build-config-reviews)
Attachment #9001851 - Flags: review?(core-build-config-reviews)
Attachment #9001852 - Flags: review?(core-build-config-reviews)
Attachment #9001854 - Flags: review?(core-build-config-reviews)
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.
Attachment #9001855 - Flags: review?(core-build-config-reviews)
Attachment #9001855 - Attachment description: 0005-Bug-1483778-Skip-LTO-during-the-profile-generate-pha.patch → Skip LTO during the profile-generate phase of PGO
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.
(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.
> 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 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...
Attachment #9001853 - Flags: review?(core-build-config-reviews) → review+
Attachment #9001851 - Flags: review?(core-build-config-reviews) → review+
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.
Attachment #9001852 - Flags: review?(core-build-config-reviews) → review+
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/
Attachment #9001854 - Flags: review?(core-build-config-reviews) → review+
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?
Attachment #9001855 - Flags: review?(core-build-config-reviews)
--rebuild-talos 10, even. And that seems to make no difference. OTOH, it's clearly faster to build.
Depends on: 1484872
Landed the already r+ed parts in bug 1484872
(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).
(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.
Attachment #9001855 - Flags: review?(nfroyd)
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?
Attachment #9001855 - Flags: review?(nfroyd) → review+
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.
Attachment #9001851 - Attachment is obsolete: true
Attachment #9001852 - Attachment is obsolete: true
Attachment #9001853 - Attachment is obsolete: true
Attachment #9001854 - Attachment is obsolete: true
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb9a0ab83ad
Skip LTO during the profile-generate phase of PGO. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/3fb9a0ab83ad
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
You need to log in before you can comment on or make changes to this bug.