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

Add support for PGO #457

Open
melix opened this issue Jun 20, 2023 · 1 comment
Open

Add support for PGO #457

melix opened this issue Jun 20, 2023 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@melix
Copy link
Collaborator

melix commented Jun 20, 2023

Now that GraalVM 23 is out with PGO support available in the Oracle GraalVM distribution, it would be good if the Gradle and Maven plugins added built-in support for PGO.

For the Gradle plugin, my recommendation would be to add a couple of options to org.graalvm.buildtools.gradle.dsl.NativeImageCompileOptions:

  • a Property<Boolean> getPgo() which can also be enabled via a CLI flag (e.g --pgo), which would enable the compilation with --pgo-instrument (and potentially add a -instrumented suffix to the image name for clarity), which would be used for the 1st step, building an image with instrumentation support
  • a DirectoryProperty getPgoProfilesDirectory() pointing to a directory of .iprof files which would be automatically added to the native image compilation if getPgo() returns false (and that there are actually files there)

We could have a conventional location of those profile files, for example src/native-image/<image name>/pgo, so for example for the main image it would be src/native-image/main/pgo (with the idea that these files would be checked in VCS).

This is not about the ability to automatically run the instrumented image with a "load test" and automating the process from A-Z, but at least to make it possible to enable PGO in a standard way.

@melix melix added the enhancement New feature or request label Jun 20, 2023
@melix melix self-assigned this Jun 20, 2023
@fniephaus
Copy link
Member

Thanks for the suggestions, @melix. I've got some comments:

a CLI flag (e.g --pgo), which would enable the compilation with --pgo-instrument

I'd like a CLI flag, but using --pgo for --pgo-instrument is probably very confusing, considering there's also a --pgo in Native Image itself. I'd still vote for --pgo-instrument even though it's longer.

and potentially add a -instrumented suffix to the image name for clarity

yes, that'd be very useful.

a DirectoryProperty getPgoProfilesDirectory() pointing to a directory of .iprof files which would be automatically added to the native image compilation if ...

Does this mean that Native Image will automatically pick up the profile stored in a default.iprof if it is in that directory? What if there are multiple profiles? Maybe users will think Native Image picks up all profiles (native-image-configure supports merging multiple profiles for example), while it really only picks up the default.iprof? Or should/could the plugin run the merge tool and feed the result into Native Image?

for the main image it would be src/native-image/main/pgo

maybe pgo-profiles or pgo-data? just an idea :)

This is not about the ability to automatically run the instrumented image.

I'm actually wondering how the iprof files find their way into this new directory for PGO profiles? We probably should provide users with the right /path/to/image-instrumented -XX:ProfilesDumpFile=full/path/to/src/native-image/main/pgo command and instructions to make sure the profiles end up in the right directory?

This is not about the ability to automatically run the instrumented image.

Quoting this a second time for another thought: why is this not about the ability to automatically run the instrumented image? I mean the plugins already support running with the tracing agent, couldn't it similarly run the instrumented binary, add the right -XX:ProfilesDumpFile flag, and tell the user to kill the app when done?

melix added a commit that referenced this issue Jul 21, 2023
melix added a commit that referenced this issue Jul 27, 2023
* Add support for PGO instrumentation

This commit adds support for PGO instrumentation. This should be
enabled by adding the `--pgo-instrument` option to the Gradle
command line. When this is done, then the generated binary will
be compiled with PGO instrumentation enabled, and the binary
name will be suffixed with `-instrumented`.

It is possible to run the instrumented binary directly too,
in which case the profile files will be written in the same
directory as the binary.

* Add support for a PGO profiles directory

By convention, the directory is set to `src/pgo-profiles/<binary>`.
For example, for the `main` binary, the directory where to put
PGO profiles would be `src/pgo-profiles/main`. If that directory
is present _and that we're not instrumenting_, then the profile
will be used when compiling with native image.

It is possible to provide multiple profiles in a single directory.

* Remove GraalVM version from workflows

* Add documentation about PGO support

See #457

* Fix JUnit native test

* Make checkstyle happy

* Fix tests

* Temporarily(?) disable testing with config cache

As we're not compatible. Test `org.graalvm.buildtools.gradle.OfficialMetadataRepoFunctionalTest`
throws an incomprehensible error message, in all versions of Gradle I've tested:

```
Configuration cache state could not be cached: field `spec` of `org.gradle.api.internal.tasks.execution.SelfDescribingSpec` bean found in task `:compileJava` of type `org.gradle.api.tasks.compile.JavaCompile`: error writing value of type 'org.gradle.api.internal.tasks.compile.CompilerForkUtils$$Lambda$1235/0x00000008015b1c38'
> Unable to make field private final java.lang.Object[] java.lang.invoke.SerializedLambda.capturedArgs accessible: module java.base does not "opens java.lang.invoke" to unnamed module @3cc98b0c
```

This PR also rewrote some code which fixed other configuration
cache issues which arose _before_ reaching this one.

* Upgrade to JUnit 5.10.0

* Make checkstyle happy

* Fix test

* Restore configuration cache tests

* Update baseline versions for config cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants