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

[GR-51629] Add support for JFR -XX:FlightRecorderOptions #7585

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

roberttoyonaga
Copy link
Collaborator

@roberttoyonaga roberttoyonaga commented Oct 10, 2023

Summary

-XX:FlightRecorderOptions can be passed on the command line when launching your app. This PR adds support for the following JFR options in native image:

  • globalbuffercount
  • globalbuffersize
  • maxchunksize
  • memorysize
  • repositorypath
  • stackdepth
  • thread_buffer_size
  • preserveRepository.

A few of these options are legacy options are are not very important (although the current OpenJDK still supports them). maxchunksize, repositorypath, andstackdepth are probably the most useful options and advanced JFR users may find them important. preserveRepository is a new option added to JDK22 via openjdk/jdk#13111.

See issue: #7546

Details

In HotSpot, the VM-level JFR options may not be changed after JFR is initialized. This means we expect that com.oracle.svm.core.jfr.JfrOptionSet will not be needed after JFR is initialized. Thus the data flow should look something like the diagram below:
HotSpot:

SubstrateVM:

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 10, 2023
@roberttoyonaga roberttoyonaga self-assigned this Oct 10, 2023
Copy link
Contributor

@galderz galderz left a comment

Choose a reason for hiding this comment

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

Small typo. Have you tested this options somehow?

Even though present in OpenJDK, I have doubts about bringing in legacy options.


globalbuffercount (Optional) Number of global buffers. This option is a legacy
option: change the memorysize parameter to alter the number of
global buffers. This value cannot be changed once JFR has been"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove training "

@roberttoyonaga
Copy link
Collaborator Author

Thanks @galderz !

Small typo.

Good catch, I'll fix that.

Have you tested this options somehow?

Yes I've tested them manually by launching JFR, passing FlightRecorderOptions and checking to make sure the specified options are respected. I'm not sure if it's possible to add a JUnit test for this because it requires passing the options when launching the executable (options must not be changed after initialization). This is a problem because JFR is started/initialized once for all the Native Image unit tests.

Even though present in OpenJDK, I have doubts about bringing in legacy options.

Yes, that's a fair point. I only decided to add them because we already implicitly handle those 'legacy" options, we just don't yet parse them from the command line. I'm fine with removing those legacy options from this PR. Let's see what @christianhaeubl thinks too.

@fniephaus
Copy link
Member

I'm not sure if it's possible to add a JUnit test for this because it requires passing the options when launching the executable (options must not be changed after initialization). This is a problem because JFR is started/initialized once for all the Native Image unit tests.

We probably need a different kind of test. Take a look at, for example, how the debuginfotest is implemented. You could generate a simple native image, then run it with the right flags in the background and do what you need to do.

I'm fine with removing those legacy options from this PR.

I'm inclined to say we don't support legacy options, unless there's actual user demand.

@christianhaeubl
Copy link
Member

I'm inclined to say we don't support legacy options, unless there's actual user demand.

Most of those legacy options are more like experimental options (i.e., they can be used to provide workarounds or fine tune JFR), so I would suggest that we support them as well and change the documentation accordingly.

I will probably start integrating this PR later today, as it has some overlap with #6743, which I am currently integrating. @roberttoyonaga: if you plan to add any test cases, please do that in a separate PR.

@fniephaus fniephaus changed the title Add support for JFR -XX:FlightRecorderOptions [GR-51629] Add support for JFR -XX:FlightRecorderOptions Jan 25, 2024
@christianhaeubl
Copy link
Member

christianhaeubl commented Jan 25, 2024

@roberttoyonaga: I opened #8254. I simplified FlightRecorderOptionsHelp.txt a bit. Please let me know if you think that I removed anything relevant.

@roberttoyonaga
Copy link
Collaborator Author

I opened #8254. I simplified FlightRecorderOptionsHelp.txt a bit. Please let me know if you think that I removed anything relevant.

Thanks @christianhaeubl. Your changes look good to me.

@graalvmbot graalvmbot merged commit 74e2175 into oracle:master Jan 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
native-image native-image-jfr OCA Verified All contributors have signed the Oracle Contributor Agreement. redhat-interest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants