Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

firefox: add ltoSupport and enable it by default #99922

Merged
merged 3 commits into from Oct 20, 2020
Merged

Conversation

@S-NA
Copy link
Contributor

@S-NA S-NA commented Oct 7, 2020

Motivation for this change

Build Firefox with LTO support as upstream does. This change switches the compiler to clang and linker to lld.

The closure size increased by ~1% (586.6M -> 592.9M).

The resulting directory of interest did decrease in size (181M -> 179M).

# Non-LTO build
$ du -sh /nix/store/q4295z228mic5c062ha6v6dyzjnf78vd-firefox-unwrapped-81.0/lib/firefox
181M    /nix/store/q4295z228mic5c062ha6v6dyzjnf78vd-firefox-unwrapped-81.0/lib/firefox

# LTO build
$ du -sh /nix/store/7qi4yl0f44zxd3lmka6yba0f2wdxa4nd-firefox-unwrapped-81.0/lib/firefox
179M    /nix/store/7qi4yl0f44zxd3lmka6yba0f2wdxa4nd-firefox-unwrapped-81.0/lib/firefox
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@dasJ
Copy link
Member

@dasJ dasJ commented Oct 7, 2020

cc @ajs124

Copy link
Member

@andir andir left a comment

This actually looks good! I'll give it a test spin on our current firefox variants.

@dasJ dasJ requested a review from flokli Oct 7, 2020
@andir
Copy link
Member

@andir andir commented Oct 7, 2020

Each of the did build on my machine and appear to be working fine. I've not done any kind of benchmarks. The builds seem to take about 1 minute longer than usually on my 24 cores which is probably fine.

@andir andir force-pushed the S-NA:wip/firefox-lto branch from 249a7bd to 4f12bdd Oct 7, 2020
@andir
Copy link
Member

@andir andir commented Oct 7, 2020

I rebased this to solve the merge conflict that I caused in another PR.

@ofborg ofborg bot requested a review from andir Oct 7, 2020
@S-NA
Copy link
Contributor Author

@S-NA S-NA commented Oct 8, 2020

Using the Speedometer 2.0 on an Intel Core 2 Duo CPU T7250 @ 2.00GHz.

Benchmark output

firefox-original:

Iteration 1	22.64 runs/min
Iteration 2	22.29 runs/min
Iteration 3	22.03 runs/min
Iteration 4	22.31 runs/min
Iteration 5	21.76 runs/min
Iteration 6	22.39 runs/min
Iteration 7	22.60 runs/min
Iteration 8	21.51 runs/min
Iteration 9	21.95 runs/min
Iteration 10	22.28 runs/min
Arithmetic Mean: 22.2 ± 0.26 (1.2%)

firefox-lto:

Iteration 1	25.67 runs/min
Iteration 2	24.89 runs/min
Iteration 3	24.43 runs/min
Iteration 4	24.37 runs/min
Iteration 5	24.12 runs/min
Iteration 6	24.53 runs/min
Iteration 7	24.81 runs/min
Iteration 8	22.44 runs/min
Iteration 9	23.76 runs/min
Iteration 10	23.67 runs/min
Arithmetic Mean: 24.3 ± 0.62 (2.5%)

firefox-bin:

Iteration 1	26.70 runs/min
Iteration 2	27.38 runs/min
Iteration 3	26.27 runs/min
Iteration 4	27.67 runs/min
Iteration 5	28.08 runs/min
Iteration 6	27.07 runs/min
Iteration 7	27.58 runs/min
Iteration 8	28.34 runs/min
Iteration 9	27.78 runs/min
Iteration 10	27.47 runs/min
Arithmetic Mean: 27.4 ± 0.44 (1.6%)

The benchmark seems to show a performance increase of ~9% when going from non-LTO to LTO.
Comparing non-LTO to LTO+PGO (firefox-bin) there is a substantial performance increase of ~23%.
Comparing LTO to LTO+PGO shows LTO helps us get closer but not quite to the firefox-bin numbers, which firefox-bin providing ~13% increase over plain LTO.

This is a considerably old machine so I am not sure how accurate the benchmark results are but I can give my opinion saying it feels smoother.

S-NA added 2 commits Oct 9, 2020
LTO in general is broken on Darwin (see #19312).
@andir
Copy link
Member

@andir andir commented Oct 9, 2020

This is looking good. Will try this one more time.

Copy link
Member

@worldofpeace worldofpeace left a comment

This LGTM 👍 Thanks a ton

@flokli
Copy link
Contributor

@flokli flokli commented Oct 11, 2020

This is looking good. Will try this one more time.

This is also looking good here. @andir, did you do some more testing?

@andir
Copy link
Member

@andir andir commented Oct 11, 2020

@ofborg eval

@andir andir merged commit b6b09ac into NixOS:master Oct 20, 2020
17 checks passed
17 checks passed
tests tests
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="aecd9ab"; rev="aecd9ab1e474d99fc32eb7298fe02ebd2e03f99d"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="aecd9ab"; rev="aecd9ab1e474d99fc32eb7298fe02ebd2e03f99d"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="aecd9ab"; rev="aecd9ab1e474d99fc32eb7298fe02ebd2e03f99d"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="aecd9ab"; rev="aecd9ab1e474d99fc32eb7298fe02ebd2e03f99d"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="aecd9ab"; rev="aecd9ab1e474d99fc32eb7298fe02ebd2e03f99d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="aecd9ab"; rev="aecd9ab1e474d99fc32eb7298fe02ebd2e03f99d"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="aecd9ab"; rev="aecd9ab1e474d99fc32eb7298fe02ebd2e03f99d"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Oct 20, 2020

While this is quite an improvement, it will break reproducibility, right? (assuming it ever was reproducible)
Probably not a big deal atm, but might become an issue in the future.

@andir
Copy link
Member

@andir andir commented Oct 20, 2020

LTO should not have that much of an impact on that front. PGO might have an impact but we haven't added that yet.

@xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Oct 20, 2020

LTO should not have that much of an impact on that front. PGO might have an impact but we haven't added that yet.

Ah sorry this is just about LTO, should be fine so far then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.