1644409 - rustc link time optimization option is "fat"

Status: UNCONFIRMED

(Firefox Build System :: General, enhancement)

Description

[Sebastien Marie [:semarie]]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:56.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

in release build, rustc is used with explicit -Clto option. If I didn't mess myself, it has been introduced this way by #1386371 .

according to https://doc.rust-lang.org/cargo/reference/profiles.html#lto , specifying only -Clto is equivalent to -Clto=true , which is the same as -Clto=fat .

I wonder if -Clto=thin wouldn't be a better option: still according to cargo reference the "thin" LTO is similar to "fat", but takes substantially less time to run while still achieving performance gains similar to "fat".

in my tests, under OpenBSD, I am encountering build failure of firefox 77.0 with limited memory (~6 Go) with -Clto, but build fine with -Clto=thin.

Comment 1

[Landry Breuil (:gaston)]

Fwiw if that matters, firefox builds on OpenBSD use --enable-official-branding but not --enable-release. I have to admit im lost in all those build knobs, and i dont even know if we should add --enable-release or if its the default in beta/release tarballs.

about:buildconfig from my 78.0b2 build:

--enable-application=browser MOZILLA_OFFICIAL=1 --disable-tests --disable-optimize CC=cc CXX=c++ SO_VERSION=92.0 --with-libclang-path=/usr/local/lib --with-system-icu --with-system-nss MAKE=gmake --disable-install-strip --with-system-zlib --disable-dbus --disable-necko-wifi --enable-official-branding --enable-sandbox --disable-updater --prefix=/usr/local

Comment 2

[Sebastien Marie [:semarie]]

Attachment: lto.diff

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 9155539 [details] [diff] [review]
lto.diff

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

I assume you have reasons for not submitting patches to phabricator?  https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

This patch looks pretty obvious; I'm curious if there are any build-time performance benefits from taking it.

Comment 4

[Mike Hommey [:glandium]]

The performance difference of the resulting binary needs to be studied.

Comment 5

[Landry Breuil (:gaston)]

(In reply to Nathan Froyd [:froydnj] from comment #3)

Comment on attachment 9155539 [details] [diff] [review]
lto.diff

Review of attachment 9155539 [details] [diff] [review]:

I assume you have reasons for not submitting patches to phabricator?
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

i think a valid reason is 'for a new contributor, the phabricator workflow is dauting (to say the least) and it's much simpler to attach a patch to bugzilla' :)

personally (as a long-time part-time contributor), i'm still not comfortable with this contraption.. which requires setting up a phabricator account linked with bugzilla, MFA, a fullblown mercurial workflow (the mach setup is yet another obscure contraption)... runs away screaming

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Salut Gaston!

You can use git with phabricator.
And we have a tutorial for noob https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html ;)

Comment 7

[Sebastien Marie [:semarie]]

(In reply to Nathan Froyd [:froydnj] from comment #3)

I assume you have reasons for not submitting patches to phabricator?

I will try to look at it. but it doesn't seems very simple :-)

This patch looks pretty obvious; I'm curious if there are any build-time
performance benefits from taking it.

For me, the immediate results was to be able to build fully a release: so there is less memory pressure.
Does there are tool for measure precisely the build difference between with and without the patch ?

:glandium, regarding runtime performance, what is the method to measure the difference ?

Comment 8

[Ricky Stewart]

Is there something we can do instead with allowing people to specify a value for -Clto=, which would default to fat? I assume that when the docs say the performance gains from thin are "similar" to fat, it means you're trading a marginal amount of performance in the resulting binary for significantly better build times/memory usage. Incurring a performance regression for e.g. shippable builds seems like a non-starter, but the option would give people like :semarie the ability to build without impacting performance-critical production builds, while giving us levers to tweak to improve performance on our end (e.g. maybe we want thin LTO in certain scenarios and fat in others for different kinds of CI builds)

Of course, if the perf investigation comes back and thin LTO is not noticeably less performant than fat, then none of that is necessary.

Comment 9

[Nathan Froyd [:froydnj]]

The other alternative is to only turn on -Clto=fat for pgo-use builds, and -Clto=thin for everything else.

Comment 10

[Mike Hommey [:glandium]]

We already have a configure option for what kind of LTO to do for C/C++, and I'd argue we should be using the same kind of LTO for both C/C++ and rust. But currently we don't, so we need to know what changing that does to performance.

Comment 11

[Sebastien Marie [:semarie]]

Attachment: Bug 1644409 - use "thin" LTO instead of "fat". r?froydnj

Comment 12

[Sebastien Marie [:semarie]]

I finally managed to be able to push the one-line diff via phabricator. I still considere spending few hours a bit "overkill" for a one-line diff.

I agree that the performance impact should be measured. But How to test the performance impact (build and runtime) ?

Comment 13

[Sebastien Marie [:semarie]]

is it possible to someone to push the diff to "try" (it requires "Level 1 Commit Access" I don't have) to obtain a run for build performance analysis ? (assuming it is the right way to test it)

for now, I tried locally to see the difference. I built firefox 77.0.1 on OpenBSD with only 1 job (to ensure some reproductibility to compare). first run with unchanged build (-Clto, so lto=fat), and second run with the patch applied (lto=thin).

on the attached graph:

• lto=fat fails to build at gkrust step (ENOMEM). at this time, the max real mem=~5Go, max virt mem=~8.1Go
• for lto=thin, there is only a part of the build on the graph (the beginning with the rust part)
• for lto=thin, max real mem=~5Go, max virt mem=~3,6Go (at end of build outside the graph, I assume for final linkage). and if I remove it, it is real=2.8Go, virt=3.6Go.

Comment 14

[Sebastien Marie [:semarie]]

Attachment: memory - build with 1 job

Comment 15

[Landry Breuil (:gaston)]

Comment on attachment 9155539 [details] [diff] [review]
lto.diff

Fwiw, i can build 78.0b5 fine (on OpenBSD, of course) with this diff, and memory pressure on the system was way less than usual.

Comment 16

[Nathan Froyd [:froydnj]]

I am going to run some perf tests this weekend with and without the patch to try and see if the differences are observable.

Comment 17

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #16)

I am going to run some perf tests this weekend with and without the patch to try and see if the differences are observable.

Can you do that with PGO and cross-LTO disabled too?

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #17)

(In reply to Nathan Froyd [:froydnj] from comment #16)

I am going to run some perf tests this weekend with and without the patch to try and see if the differences are observable.

Can you do that with PGO and cross-LTO disabled too?

Why do you want those? I guess to see if local developer opt builds would suffer performance-wise if people are trying to do reasonable profiling?

Comment 19

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #18)

Why do you want those? I guess to see if local developer opt builds would suffer performance-wise if people are trying to do reasonable profiling?

That, and downstreams that don't build with PGO and/or clang.

Comment 20

[Nathan Froyd [:froydnj]]

I ran into a bunch of compilation and runtime failures that I did not investigate; I had not expected a one-line change to cause so many issues:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ec46beabce7d1e586253e59de66df642e4a50f4

Comment 21

[Mike Hommey [:glandium]]

Attachment: Bug 1644409 - Make --enable-lto=thin work with GCC.

Comment 22

[Mike Hommey [:glandium]]

Attachment: Bug 1644409 - Use thin LTO for Rust unless C/C++ is built with fat LTO.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #20)

I ran into a bunch of compilation and runtime failures that I did not investigate; I had not expected a one-line change to cause so many issues:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ec46beabce7d1e586253e59de66df642e4a50f4

I tested with and without PGO, and didn't hit these problems. Performance-wise, there doesn't seem to be a difference between -Clto and -Clto=thin. There is a difference when using neither of them, as one would expect, although it /seems/ we may get away with not using -Clto/-Clto=thin when building with cross LTO, but I'd rather do that separately.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/795c8762b16b
Make --enable-lto=thin work with GCC. r=froydnj

Comment 25

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/795c8762b16b

Comment 26

[Wolfgang Rosenauer [:wolfiR]]

I just run into an issue which might be related but not fully investigated yet.

I could compile FF80 with gcc and lto but with 81 RC it fails like this:

[ 124s] 0:23.30 DEBUG: configure:9393: checking what kind of list files are supported by the linker
[ 124s] 0:23.30 DEBUG: configure:9398: /usr/bin/ccache /usr/bin/gcc -std=gnu99 -o conftest.o -c -flto=thin -flifetime-dse=1 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe conftest.c 1>&5
[ 124s] 0:23.30 DEBUG: cc1: error: unrecognized argument to '-flto=' option: 'thin'
[ 124s] 0:23.30 DEBUG: configure: error: couldn't compile a simple C file
[ 124s] 0:23.30 ERROR: old-configure failed

Comment 27

[Cameron Kaiser [:spectre]]

Yes, I'm hitting this too.

% gcc --version
gcc (GCC) 10.2.1 20200723 (Red Hat 10.2.1-1)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Comment 28

[Cameron Kaiser [:spectre]]

(I get past it with --enable-lto=full in my .mozconfig.)

Comment 29

[Thaodan]

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #26)

I just run into an issue which might be related but not fully investigated yet.

I could compile FF80 with gcc and lto but with 81 RC it fails like this:

[ 124s] 0:23.30 DEBUG: configure:9393: checking what kind of list files are supported by the linker
[ 124s] 0:23.30 DEBUG: configure:9398: /usr/bin/ccache /usr/bin/gcc -std=gnu99 -o conftest.o -c -flto=thin -flifetime-dse=1 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe conftest.c 1>&5
[ 124s] 0:23.30 DEBUG: cc1: error: unrecognized argument to '-flto=' option: 'thin'
[ 124s] 0:23.30 DEBUG: configure: error: couldn't compile a simple C file
[ 124s] 0:23.30 ERROR: old-configure failed

Could we get an update on this? It is sad that we have to revert a patch that should have "fixed" the issue but just add something on top.

Comment 30

[Wolfgang Rosenauer [:wolfiR]]

I needed https://src.fedoraproject.org/rpms/firefox/blob/master/f/pgo.patch to build FF84 with gcc