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

chromium: build with upstream llvm & PGO #101786

Closed
wants to merge 3 commits into from

Conversation

samuelgrf
Copy link
Member

@samuelgrf samuelgrf commented Oct 26, 2020

Motivation for this change

Google reports up to 10% faster page loads.
Source: https://blog.chromium.org/2020/08/chrome-just-got-faster-with-profile.html

Can decrease overall compile time by 20%.
Source: https://llvm.org/docs/HowToBuildWithPGO.html

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.

@samuelgrf
Copy link
Member Author

So far I have only tested that the build starts without errors, I'll report back once it's finished.

@TredwellGit
Copy link
Member

I am under the impression that #101467 already does this.
https://source.chromium.org/chromium/chromium/src/+/master:build/config/compiler/pgo/pgo.gni will set chrome_pgo_phase = 2 when is_official_build == true. https://source.chromium.org/chromium/chromium/src/+/master:build/config/compiler/pgo/BUILD.gn;l=58 downloads PGO profiles.

@samuelgrf
Copy link
Member Author

samuelgrf commented Oct 26, 2020

It's not that simple unfortunately.

https://source.chromium.org/chromium/chromium/src/+/master:build/config/compiler/pgo/BUILD.gn;l=58 doesn't actually fetch any profile files, it runs the update_pgo_profiles.py script with the get_profile_path argument, which only gets existing profiles.

To enable fetching profiles https://source.chromium.org/chromium/chromium/src/+/master:DEPS;l=121 can be set to true, which would run the update script at https://source.chromium.org/chromium/chromium/src/+/master:DEPS;l=4961 .
This approach doesn't work on Nix though, since network access is disabled during the configuration and build phase, which is why I ended up with this solution.

@TredwellGit
Copy link
Member

Good catch.

@TredwellGit
Copy link
Member

@primeos

@samuelgrf
Copy link
Member Author

Result of nixpkgs-review pr 101786 1

1 package built:
  • chromium

@samuelgrf
Copy link
Member Author

For some reason nixpkgs-review only built the stable variant, the binary runs without issues.
I have manually started the beta build and it's running atm.

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Thanks! I have two nitpicks though (at least the 1st one needs to be resolved so that running the script doesn't fail - depending on the installed Python libraries). Other than that it LGTM.

For some reason nixpkgs-review only built the stable variant, the binary runs without issues.

Could be because we only built chromium on Hydra (meta.hydraPlatforms), not sure though if nixpkgs-review considers that.

@samuelgrf
Copy link
Member Author

Weirdly enough every benchmark I've tried has worse results on pgo than on master, I don't think this should be merged until that's fixed.
Does anyone have an idea why that could be?

Speedometer master: 141.4
Speedometer pgo: 130.5

Kraken master: 696.9ms
Kraken pgo: 716.7ms

Octane master: 54105
Octane pgo: 52454

@samuelgrf
Copy link
Member Author

samuelgrf commented Nov 3, 2020

I am pretty sure the performance decrease is caused by mismatching clang versions.

PGO builds are disabled in Ubuntu bionic-dev, the commit message says this was done because the upstream profile data is not compatible with the version of clang used to build chromium. Source
We are using clang 11, while upstream chromium uses clang 12. Source

Building with mismatched profile data would normally cause compiler errors, but this is disabled in the Chromium build files. Source

@samuelgrf samuelgrf closed this Nov 3, 2020
@TredwellGit
Copy link
Member

The prebuilt LLVM binaries could be used, but we would need to get depot_tools and gclient runhooks working. Or better yet, package the LLVM revisions that Chromium distributes that way we don't rely on unverified binaries.

@samuelgrf samuelgrf reopened this Nov 3, 2020
@samuelgrf samuelgrf marked this pull request as draft November 3, 2020 17:30
@samuelgrf
Copy link
Member Author

samuelgrf commented Nov 3, 2020

AFAIK network access is disabled during Nix builds.
What we could do is add the clang tarball to the update script and fetch it using fetchurl, the relevant data is inside tools/clang/scripts/update.py.
The DL link for 86.0.4240.111 would be https://commondatastorage.googleapis.com/chromium-browser-clang/Linux_x64/clang-llvmorg-12-init-3492-ga1caa302-1.tgz .

I just don't know how we'd get the binaries to run, can autoPatchelf be used for this?

@TredwellGit
Copy link
Member

Again, it would be better (and maybe easier) to generate llvmPackages for each Chromium version then set

to those versions.

@samuelgrf samuelgrf changed the title chromium: build with PGO chromium: build with upstream llvm & PGO Nov 12, 2020
@samuelgrf
Copy link
Member Author

samuelgrf commented Nov 12, 2020

Ok, I've applied the changes mentioned in the review and also implemented upstream llvm builds, nixpkgs-review builds everything without issues (excluding dev, as it's marked as broken).

Benchmark results look better now, Speedometer improved by ~5%, MS Chalkboard by ~6%, Octane 2 and Kraken by ~0.6%.

I am just not sure how to best handle the upstream llvm builds, I have a few questions (ping @primeos):

  1. Should I create a completely new directory in llvmPackages for chromium builds?
    With that solution we could remove multiple if/else statements and simplify the expression, but it'd cause lots of code duplication and the buildOverride argument might also be useful for other tools like rocm.
  2. Should I move the llvm expressions to llvm/12 instead, or should we wait for it's release first?

targetLlvmLibraries = llvmPackages.libraries;
buildOverride = upstream-info.deps.clang;
};

stdenv = llvmPackages.stdenv;
Copy link
Member

Choose a reason for hiding this comment

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

I probably should have set stdenv = llvmPackages.lldClang.stdenv in #101467. Maybe it would be good to include?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, llvmPackages.lldClang.stdenv is totally broken.

@primeos
Copy link
Member

primeos commented Nov 12, 2020

Sorry for my delayed reply (I'm a bit behind and only briefly glanced over the diff so far).

Or better yet, package the LLVM revisions that Chromium distributes that way we don't rely on unverified binaries.

Yes, we should avoid unverified binaries and build as much from source as possible (personally I'm completely against pre-built binaries).

  1. Should I create a completely new directory in llvmPackages for chromium builds?

Good question :o IIRC Chromium uses LLVM master, I'm not sure how often they update it though.
https://chromium.googlesource.com/chromium/src/+/master/docs/clang.md states:

It's just upstream clang built at a known-good revision that we bump every two weeks or so.

First, I'd hope they don't update LLVM during patch releases for the stable channel (though IIRC it doesn't take that long to compile, especially vs. Chromium of course). But at least for Chromium dev the LLVM is probably bumped every two weeks and might be enough ahead of the one for the stable channel that it causes problems (i.e. doesn't build). So we could introduce pkgs/development/compilers/llvm/latest but pkgs/development/compilers/llvm/chromium might be more appropriate.

In any case we should also involve the LLVM maintainers regarding this (IIRC they also considered switching to the monorepo build). Another problem is that the LLVM will probably break after some new updates for the beta/dev channel and we should have a dedicated maintainer to take care of this (I'm already busy enough with Chromium :o).

@samuelgrf
Copy link
Member Author

Closing because I don't have the time to maintain the custom LLVM builds.

@samuelgrf samuelgrf closed this Mar 29, 2021
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

4 participants