-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
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.
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" |
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.
Remove training "
Thanks @galderz !
Good catch, I'll fix that.
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.
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. |
add repositorypath option
4163a38
to
74e2175
Compare
We probably need a different kind of test. Take a look at, for example, how the
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. |
@roberttoyonaga: 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. |
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: