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

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 commented Oct 7, 2020

cc @ajs124

Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dasJ dasJ requested a review from flokli October 7, 2020 11:04
@andir
Copy link
Member

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
Copy link
Member

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 October 7, 2020 14:13
@S-NA
Copy link
Contributor Author

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.

@andir
Copy link
Member

andir commented Oct 9, 2020

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

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM 👍 Thanks a ton

@flokli
Copy link
Contributor

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 commented Oct 11, 2020

@ofborg eval

@andir andir merged commit b6b09ac into NixOS:master Oct 20, 2020
@xaverdh
Copy link
Contributor

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 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 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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants