-
Couldn't load subscription status.
- Fork 6.1k
8369736 - Add management interface for AOT cache creation #28010
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
base: pr/27965
Are you sure you want to change the base?
Conversation
|
👋 Welcome back macarte! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| * </blockquote> | ||
| * | ||
| * @return {@code true} if a recording was in progress and has been ended successfully; {@code false} otherwise. | ||
| */ |
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.
There are issues in the javadoc that will cause the docs target to fail, e.g. mismatched blockquote. Can you fix these up so that we can generate the docs from the changes in the PR?
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.
Is there an easy way to test the correctness of the javadoc parts (I couldn't find an option for javadoc). The </blockquote >above is matched to a <blockquote> on line 88, but there is a <pre></pre> pair in between, is that the issue you are referring to?
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.
make docs. The second blockquote is matched with another blockquote, not </blockquote>. In the example, which can be converted to a snippet, then then braces not matched so the end brace for {@code ... } get mixed up.
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.
I see that now - fixing ....
|
|
||
| import java.lang.management.ManagementFactory; | ||
| import java.lang.management.PlatformManagedObject; | ||
| import java.util.concurrent.ForkJoinPool; |
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.
I assume there is no need to import ForkJoinPool.
| /** | ||
| * Management interface for the JDK's Ahead of Time (AOT) optimizations. | ||
| * | ||
| * Currently, {@code HotSpotAOTCacheMXBean} defines one operation at this time to end the AOT recording. |
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.
I think you drop "Currently, ", it will be a bit clearer without it.
| class HotSpotAOTCacheMXBeanApp { | ||
| public static void main(String[] args) { | ||
| System.out.println("Hello Leyden " + args[0]); | ||
| var aotBean = ManagementFactory.getPlatformMXBean(HotSpotAOTCacheMXBean.class); |
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.
This uses MF.getPlatformMXBean in the child VM. A more complete test would repeat with:
MBeanServer server = ManagementFactory.getPlatformMBeanServer();
HotSpotAOTCacheMXBean bean = ManagementFactory.newPlatformMXBeanProxy(server,
"jdk.management:type=HotSpotAOTCacheMXBean",
HotSpotAOTCacheMXBean.class);
In any case, it might be simpler to test that the MXBean is registered in the test itself, it doesn't need to be in the child VM.
| import javax.management.ObjectName; | ||
|
|
||
| /** | ||
| * Management interface for the JDK's Ahead of Time (AOT) optimizations. |
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.
I don't like the word "optimizations" here but don't have a better one. Maybe "operation"? Still not great
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.
Well, one possible alternative is kind of implicit in the bean name:
"Management interface for the JDK's Ahead of Time (AOT) Cache"
src/jdk.management/share/classes/jdk/management/HotSpotAOTCacheMXBean.java
Outdated
Show resolved
Hide resolved
src/jdk.management/share/classes/jdk/management/HotSpotAOTCacheMXBean.java
Outdated
Show resolved
Hide resolved
…eMXBean.java Co-authored-by: Dan Heidinga <[email protected]>
…eMXBean.java Co-authored-by: Dan Heidinga <[email protected]>
Add jdk.management.AOTCacheMXBean. The interface provides a single action that when called will cause any hosted JVM currently recording AOT information will stop recording. Existing functionality is preserved: when stopped the JVM will create the required artifacts based on the execution mode. Conveniently as the application running on the JVM has not stopped (as was previously the only way to stop recording), the application will resume execution after the artifacts have been generated.
The interface will return TRUE if a recording was successfully stopped, in all other cases (not recording etc.) will return FALSE
It follows that invoking the action on a JVM that is recording, twice in succession, should (baring internal errors) produce the following two responses:
TRUE
FALSE
Passes tier1 on linux (x64) and windows (x64)
Progress
Integration blocker
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28010/head:pull/28010$ git checkout pull/28010Update a local copy of the PR:
$ git checkout pull/28010$ git pull https://git.openjdk.org/jdk.git pull/28010/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28010View PR using the GUI difftool:
$ git pr show -t 28010Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28010.diff
Using Webrev
Link to Webrev Comment