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
Conversation
cc @ajs124 |
There was a problem hiding this 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.
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. |
I rebased this to solve the merge conflict that I caused in another PR. |
Using the Speedometer 2.0 on an Intel Core 2 Duo CPU T7250 @ 2.00GHz. Benchmark output
firefox-original:
firefox-lto:
firefox-bin:
The benchmark seems to show a performance increase of ~9% when going from non-LTO to 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. |
LTO in general is broken on Darwin (see NixOS#19312).
This is looking good. Will try this one more time. |
There was a problem hiding this 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
This is also looking good here. @andir, did you do some more testing? |
@ofborg eval |
While this is quite an improvement, it will break reproducibility, right? (assuming it ever was reproducible) |
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 |
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).
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)