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 cmdline opts to toggle extendedHCR #19554

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Conversation

tajila
Copy link
Contributor

@tajila tajila commented May 24, 2024

Disable HCR by default. Add option to enabled it
if needed. Add warning message to indicate a
redefinition of retransformation failure caused by the lack of extended HCR support.

@tajila
Copy link
Contributor Author

tajila commented May 24, 2024

@pshipton is there a compile time flag that lets us distinguish between IBM Java8 and Semeru8?

@pshipton
Copy link
Member

#if defined(OPENJ9_BUILD)

@pshipton
Copy link
Member

pshipton commented May 24, 2024

What you could do is not distinguish in the VM, but add the option to enable for IBM Java 8 in options.default. That's what we've done for a few other things.

-Djava.lang.stringBuffer.growAggressively=false
-XX:+OriginalJDK8HeapSizeCompatibilityMode
-XX:+LegacyXlogOption

@gacholio
Copy link
Contributor

What is the point of this? We only go into extended mode for FSD (i.e. when certain JVMTI capabilities are requested), so this would have no impact on an actual application that is not running in debug mode.

@tajila
Copy link
Contributor Author

tajila commented May 25, 2024

What is the point of this?

I want to disable extended HCR by default so we can have guarantees about ramClass addresses.

We only go into extended mode for FSD (i.e. when certain JVMTI capabilities are requested), so this would have no impact on an actual application that is not running in debug mode.

If someone is running in debug mode and it fails because we have explicitly disabled extended HCR, I'd like them to get a warning message indicating that this is the case.

@gacholio
Copy link
Contributor

This seems like a waste of code. Just return false from areExtensionsEnabled if the flag is set. Printing to the console is a waste of time (and you would need to NLS the messages). JVMTI has error codes that detail why the HCR failed - that should be sufficient for a feature that no one uses.

@gacholio
Copy link
Contributor

If we're not removing the extensions entirely, then we will still need to maintain the related code paths, so I'm still not convinced this is useful.

@tajila
Copy link
Contributor Author

tajila commented May 27, 2024

This seems like a waste of code. Just return false from areExtensionsEnabled if the flag is set. Printing to the console is a waste of time (and you would need to NLS the messages). JVMTI has error codes that detail why the HCR failed - that should be sufficient for a feature that no one uses.

The issue I am trying to address is the case where someone is currently relying on extensions, and then gets a jvmti error. If they haven't changed their application or setup then any failure would be surprising. So the error message helps a user quickly mitigate the issue.

If we're not removing the extensions entirely, then we will still need to maintain the related code paths, so I'm still not convinced this is useful.

Long term I'd like to remove it. This is a first step in doing so. As part of this change I'd like to announce deprecation for future removal.

We have no idea if anyone is using the feature. So I think having some kind of warning message is the safest route forward.

runtime/util/hshelp.c Outdated Show resolved Hide resolved
@tajila tajila force-pushed the ehcr branch 8 times, most recently from 9666656 to 5c41f4d Compare May 27, 2024 18:59
@tajila
Copy link
Contributor Author

tajila commented May 27, 2024

jenkins test sanity zlinux jdk17

runtime/oti/jvminit.h Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor

I suspect there will need to be changes to some tests. I'll run a sanity build now to see what fails.

@gacholio
Copy link
Contributor

jenkins test sanity zlinux jdk8

@gacholio
Copy link
Contributor

Looks like we only have one test for extended HCR which will need the command line option added:

TestRefreshGCSpecialClassesCache_BCI_EXTENDED_HCR_SE80_0

@tajila
Copy link
Contributor Author

tajila commented May 28, 2024

jenkins test sanity,extended zlinux jdk8

@tajila
Copy link
Contributor Author

tajila commented May 29, 2024

jenkins test extended zlinux jdk8

@tajila
Copy link
Contributor Author

tajila commented May 30, 2024

jenkins test sanity,extended zlinux jdk8

runtime/util/hshelp.c Outdated Show resolved Hide resolved
@tajila
Copy link
Contributor Author

tajila commented May 31, 2024

jenkins test extended zlinux jdk8

@tajila tajila force-pushed the ehcr branch 2 times, most recently from 7fc53a9 to 3dbccba Compare May 31, 2024 20:35
@tajila
Copy link
Contributor Author

tajila commented May 31, 2024

jenkins test extended zlinux jdk8

1 similar comment
@tajila
Copy link
Contributor Author

tajila commented Jun 3, 2024

jenkins test extended zlinux jdk8

runtime/vm/jvminit.c Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor

gacholio commented Jun 6, 2024

Is the options.default file processed for OpenJ9? If so, you may need to ifdef the default bit set.

@pshipton
Copy link
Member

pshipton commented Jun 6, 2024

options.default is used by OpenJ9. The idea is to have the code set the default for OpenJ9 builds, and then modify the options.default for IBM Java 8 (in closedj9) to override the default by using the option.

Disable HCR by default. Add option to enabled it
if needed. Add warning message to indicate a
redefinition of retransformation failure caused by
the lack of extended HCR support.

Signed-off-by: Tobi Ajila <[email protected]>
@tajila tajila marked this pull request as ready for review June 10, 2024 16:06
@gacholio
Copy link
Contributor

jenkins test sanity,extended zlinux jdk21

@gacholio
Copy link
Contributor

jenkins compile win jdk8

@gacholio
Copy link
Contributor

I won't merge this until the closedj9 change is made.

@tajila
Copy link
Contributor Author

tajila commented Jun 11, 2024

I won't merge this until the closedj9 change is made.

closedj9 changes are merged

@gacholio gacholio merged commit 775a2c2 into eclipse-openj9:master Jun 11, 2024
7 checks passed
@pshipton
Copy link
Member

Pls don't forget to open the docs issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants