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.

Sign up for GitHub

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

Jump to bottom

Support HotSpotDiagnosticMXBean in OpenJ9 JDK21+ #17709

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Support HotSpotDiagnosticMXBean in OpenJ9 JDK21+ #17709

merged 1 commit into from
Jun 30, 2023

Conversation

babsingh
Copy link
Contributor

This will allow us to enable a JTReg test for Structured Concurrency.

JTReg test: StructuredTaskScope/StructuredThreadDumpTest

Related: #17634

@babsingh
Copy link
Contributor Author

@pshipton @tajila I verified that StructuredThreadDumpTest passes with this change. Also, I didn't observe any side-effects of this change in personal builds. Can one of you please review these changes?

@pshipton
Copy link
Member

It seems this will enable other HotSpotDiagnosticMXBean APIs which aren't going to work on OpenJ9. What happens when they are called? We may need to do something so they fail gracefully, such as throw an appropriate exception.

https://download.java.net/java/early_access/jdk21/docs/api/jdk.management/com/sun/management/HotSpotDiagnosticMXBean.html

getDiagnosticOptions()
getVMOption()
setVMOption()

@tajila
Copy link
Contributor

tajila commented Jun 29, 2023

Does the use of HotSpotDiagnosticMXBean require -XX:+UnlockDiagnosticVMOptions on hotspot?

@babsingh
Copy link
Contributor Author

It seems this will enable other HotSpotDiagnosticMXBean APIs which aren't going to work on OpenJ9. What happens when they are called? We may need to do something so they fail gracefully, such as throw an appropriate exception.

https://download.java.net/java/early_access/jdk21/docs/api/jdk.management/com/sun/management/HotSpotDiagnosticMXBean.html

getDiagnosticOptions() getVMOption() setVMOption()

Initially, I thought it was only Java code. After running some JTReg tests, these methods also rely upon native methods which are not implemented in OpenJ9. So, I will add an override similar to dumpHeap, which will throw an UnsupportedOperationException.

@babsingh
Copy link
Contributor Author

Does the use of HotSpotDiagnosticMXBean require -XX:+UnlockDiagnosticVMOptions on hotspot?

I don't believe that it requires -XX:+UnlockDiagnosticVMOptions. StructuredThreadDumpTest uses HotSpotDiagnosticMXBean without -XX:+UnlockDiagnosticVMOptions.

@babsingh babsingh force-pushed the support_HotSpotDiagnosticMXBean branch 2 times, most recently from 4dd830d to 46460ea Compare June 29, 2023 17:21
@babsingh
Copy link
Contributor Author

All feedback comments have been addressed.

@babsingh babsingh force-pushed the support_HotSpotDiagnosticMXBean branch from 46460ea to 3c5d345 Compare June 29, 2023 21:08
@babsingh
Copy link
Contributor Author

Previous feedback has been addressed: removed the module check and added error handling as per the Javadoc.

@pshipton
Copy link
Member

What test suite(s) did you run already?

@babsingh babsingh force-pushed the support_HotSpotDiagnosticMXBean branch from 3c5d345 to 5daea5b Compare June 29, 2023 21:44
@babsingh
Copy link
Contributor Author

babsingh commented Jun 29, 2023 •
edited
Loading

What test suite(s) did you run already?

JDK21: sanity.functional,sanity.openjdk,extended.openjdk. JDK21 personal build: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/17143/

JDK17: sanity.functional. JDK17 personal build: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/17144/

Ran the disabled test locally.

Also, accounted for the recent feedback: an empty list is returned and exception messages have been updated.

@pshipton
Copy link
Member

jenkins test extended amac jdk11,jdk21

@babsingh
Support HotSpotDiagnosticMXBean in OpenJ9 JDK21+
346a572 
This will allow us to enable a JTReg test for Structured Concurrency.

JTReg test: StructuredTaskScope/StructuredThreadDumpTest

Related: eclipse-openj9#17634

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh force-pushed the support_HotSpotDiagnosticMXBean branch from 5daea5b to 346a572 Compare June 30, 2023 13:54
@babsingh
Copy link
Contributor Author

babsingh commented Jun 30, 2023 •
edited
Loading

Old PR builds: https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/4025/. No issues seen during local testing with the latest changes.

@pshipton pshipton merged commit 2ea9da2 into eclipse-openj9:master Jun 30, 2023
keithc-ca
keithc-ca reviewed Jun 30, 2023
View reviewed changes
...src/java.management/share/classes/com/ibm/java/lang/management/internal/ManagementUtils.java
Comment on lines -773 to -775
create(OPENJ9_DIAGNOSTICS_MXBEAN_NAME, openj9.lang.management.internal.OpenJ9DiagnosticsMXBeanImpl.getInstance())
.addInterface(openj9.lang.management.OpenJ9DiagnosticsMXBean.class)
.validateAndRegister();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please help me understand why this was removed. This code is specific to Java 8 where there's no possibility that OpenJ9DiagnosticsMXBeanImpl.getInstance() might return null (even though the code would properly handle that situation). Why do we no longer want this bean to be registered?

Copy link
Member

Choose a reason for hiding this comment

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

Ah good catch, I missed it was specific to 8 (and we didn't test 8). So we need to add it back and add the HotspotBean there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #17726 to readd the registration calls.

@babsingh babsingh mentioned this pull request Jun 30, 2023
Merged
babsingh added a commit to babsingh/openj9 that referenced this pull request Jun 30, 2023
@babsingh
Restore OpenJ9DiagnosticsMXBean registration call in ManagementUtils
cbb2e93 
The registration call was removed in eclipse-openj9#17709 due to a
false assumption. It has been added back since it is still required.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
babsingh added a commit to babsingh/aqa-tests that referenced this pull request Jul 6, 2023
@babsingh
[JDK21] Enable StructuredThreadDumpTest for OpenJ9
4174f97 
eclipse-openj9/openj9#17709 adds the required support to enable
StructuredThreadDumpTest.

Related issues which can be closed after the test is enabled:
- eclipse-openj9/openj9#17634
- eclipse-openj9/openj9#15278

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh mentioned this pull request Jul 6, 2023
Merged
llxia pushed a commit to adoptium/aqa-tests that referenced this pull request Jul 6, 2023
@babsingh
[JDK21] Enable StructuredThreadDumpTest for OpenJ9 (#4659)
902f381 
eclipse-openj9/openj9#17709 adds the required support to enable
StructuredThreadDumpTest.

Related issues which can be closed after the test is enabled:
- eclipse-openj9/openj9#17634
- eclipse-openj9/openj9#15278

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
This was referenced Jul 6, 2023
Closed
Closed
@babsingh babsingh mentioned this pull request Jul 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca left review comments

@pshipton pshipton pshipton approved these changes

Assignees
No one assigned
Labels
comp:vm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@babsingh @pshipton @tajila @keithc-ca