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

Build Turbine native image with PGO #22197

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 30, 2024

The Turbine native image is now built with GraalVM Enterprise, making use of its profile-guided optimization capability. A script generates a PGO profile by running Turbine on a representative Java target (currently skyframe_cluster).

On Linux x86_64, this improves the overall wall time of the benchmark by ~10%:

===== Benchmarking prebuilt Turbine =====
Benchmark 1: build_target
  Time (mean ± σ):     51.813 s ±  0.900 s    [User: 0.017 s, System: 0.008 s]
  Range (min … max):   50.795 s … 52.931 s    5 runs
 
===== Benchmarking Turbine built from HEAD =====
Benchmark 1: build_target
  Time (mean ± σ):     45.898 s ±  0.683 s    [User: 0.012 s, System: 0.012 s]
  Range (min … max):   45.063 s … 46.776 s    5 runs

On macOS with an M3 Max, performance remains mostly unchanged (though without PGO, the binary built from HEAD is slightly slower than the prebuilt one, so in total there could be a somewhat more noticeably improvement):

===== Benchmarking prebuilt Turbine =====
Benchmark 1: build_target
  Time (mean ± σ):     16.928 s ±  0.167 s    [User: 0.009 s, System: 0.010 s]
  Range (min … max):   16.688 s … 17.080 s    5 runs

===== Benchmarking Turbine built from HEAD =====
Benchmark 1: build_target
  Time (mean ± σ):     16.767 s ±  0.161 s    [User: 0.009 s, System: 0.011 s]
  Range (min … max):   16.571 s … 16.994 s    5 runs

With --jobs=4, the M3 Max reproduces a similar 10% improvement in build wall time, indicating that at a higher level of parallelism Turbine performance doesn't really have an impact on wall time anymore.

When manually building //src/main/java/com/google/devtools/build/lib/cmdline with Turbine with and without PGO, the M3 Max shows a significant improvement:

Regular java_binary:
  Time (mean ± σ):     317.1 ms ±   5.3 ms    [User: 939.3 ms, System: 202.3 ms]
  Range (min … max):   305.7 ms … 341.8 ms    40 runs
GraalVM CE:
  Time (mean ± σ):      61.7 ms ±   0.9 ms    [User: 49.0 ms, System: 9.8 ms]
  Range (min … max):    60.4 ms …  65.6 ms    46 runs
Oracle GraalVM, without PGO:
  Time (mean ± σ):      40.6 ms ±   0.7 ms    [User: 31.8 ms, System: 6.6 ms]
  Range (min … max):    39.2 ms …  42.9 ms    68 runs
Oracle GraalVM, with PGO:
  Time (mean ± σ):      34.0 ms ±   0.8 ms    [User: 25.8 ms, System: 6.1 ms]
  Range (min … max):    32.4 ms …  37.6 ms    82 runs

This commit also bundles a few fixes for the benchmark and Turbine build:

  • Updates apple_support, which provides the toolchain used to build Turbine on macOS.
  • Adds a workaround for Spuriously breakage in Gerrit CI after upgrading from 7.0.0rc2 to 7.0.0rc3 #20161 to the benchmark.
  • Updates the benchmark to align its Java toolchain's source/target versions with those of Bazel, which were bumped since the benchmark was added, thus breaking its toolchain selection.
  • Raise the number of warmup runs in the benchmark to reduce the risk of statistical outliers observed during runs on macOS.
  • Remove GraalVM flags already set by rules_graalvm.
  • Add --strict-image-heap to opt into stricter GraalVM behavior that will become the default in the future.

@fmeum fmeum requested a review from cushon April 30, 2024 17:10
@fmeum fmeum marked this pull request as ready for review April 30, 2024 17:10
@fmeum fmeum requested a review from a team as a code owner April 30, 2024 17:10
@github-actions github-actions bot added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Apr 30, 2024
Copy link
Collaborator Author

@fmeum fmeum Apr 30, 2024

Choose a reason for hiding this comment

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

This file is pretty large and could potentially be used to hide exploits (I promise I didn't :-)). It would be great if this could be regenerated on import, followed by a rerun of the benchmark.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this as a way to get started. Did you have ideas for updating this automatically with CI, or was the idea for now that whoever does the import could regenerate it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could probably replace the bazel run script with an aspect that runs a modified Turbine action to collect the profile as an output file. But CI has a lot of other stuff running in parallel, so profiling may not lead to accurate results anymore. Profile generation is highly non-reproducible. Alternatively, we could run this in the release pipeline for java_tools.

@fmeum fmeum marked this pull request as draft May 2, 2024 07:06
@fmeum fmeum marked this pull request as ready for review May 4, 2024 17:32
@fmeum
Copy link
Collaborator Author

fmeum commented May 10, 2024

@iancha1992 Could you add awaiting-merge?

@sgowroji sgowroji added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 10, 2024
@cushon
Copy link
Contributor

cushon commented May 22, 2024

I'm working on getting this imported, but there are some details I need to work through internally and it might take another couple of weeks, sorry for the delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants