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 configure support to enable RAM class persistence #770

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

ThanHenderson
Copy link
Contributor

Adds support for setting the J9VM_OPT_SNAPSHOTS directive with the --enable-snapshots configure option. This enables the ongoing RAMClass persistence work.

Related: https://github.ibm.com/runtimes/openj9/issues/755
Signed-off-by: Nathan Henderson [email protected]

@ThanHenderson
Copy link
Contributor Author

ping @babsingh

@keithc-ca
Copy link
Member

Please correct the typo in the commit message and the summary here to spell "configure" correctly.

@ThanHenderson ThanHenderson changed the title Add congifure support to enable RAM class persistence Add configure support to enable RAM class persistence Oct 18, 2024
closed/OpenJ9.gmk Outdated Show resolved Hide resolved
@babsingh
Copy link
Member

Jenkins compile aix,osx,win,zlinux jdk8

@babsingh babsingh merged commit 11d7c51 into ibmruntimes:openj9 Oct 22, 2024
7 checks passed
@keithc-ca
Copy link
Member

Neither the UMA flag opt_snapshots nor the cmake build configuration flag J9VM_OPT_SNAPSHOTS exist: How can this have any effect?

Why was it not first proposed for the head stream and then back-ported to earlier versions?

AC_MSG_RESULT([yes (explicitly enabled)])
OPENJ9_ENABLE_SNAPSHOTS=true
elif test "x$enable_snapshots" = xno ; then
AC_MSG_RESULT([no (explicit)])
Copy link
Member

Choose a reason for hiding this comment

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

Why does this not say "no (explicitly disabled)" like other options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This is a mistake. I will open new PR to address any concerns you have here.

@@ -61,6 +61,7 @@ OPENJ9_ENABLE_DEMOS := @OPENJ9_ENABLE_DEMOS@
OPENJ9_ENABLE_JFR := @OPENJ9_ENABLE_JFR@
OPENJ9_ENABLE_JITSERVER := @OPENJ9_ENABLE_JITSERVER@
OPENJ9_ENABLE_OPENJDK_METHODHANDLES := @OPENJ9_ENABLE_OPENJDK_METHODHANDLES@
OPENJ9_ENABLE_SNAPSHOTS := @OPENJ9_ENABLE_SNAPSHOTS@
Copy link
Member

Choose a reason for hiding this comment

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

Tabs should only be used for indentation (not after any non-whitespace character).

FEATURE_SED_SCRIPT += $(call SedEnable,opt_snapshots)
FEATURE_SED_SCRIPT += $(call SedDisable,opt_snapshots)
else # OPENJ9_ENABLE_SNAPSHOTS
FEATURE_SED_SCRIPT += $(call SedEnable,opt_snashots)
Copy link
Contributor Author

@ThanHenderson ThanHenderson Oct 28, 2024

Choose a reason for hiding this comment

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

Just noticed this is incorrect too. I had forgotten to push the most recent changes before this was merged.

@ThanHenderson
Copy link
Contributor Author

Neither the UMA flag opt_snapshots nor the cmake build configuration flag J9VM_OPT_SNAPSHOTS exist: How can this have any effect?

J9VM_OPT_SNAPSHOTS is added in the accompanying OpenJ9 patch. (we don't need opt_snapshots; its presence here is a mistake)

Why was it not first proposed for the head stream and then back-ported to earlier versions?

It is part of a revival of an older project that was/is only currently supporting JDK8.

@keithc-ca
Copy link
Member

Reference to eclipse-openj9/openj9#20387 and an explanation why it only applies to jdk8 would have been welcome pieces of information to include in the description here (although I have trouble imagining why it needs to be restricted to jdk8).

@ThanHenderson
Copy link
Contributor Author

Apologies, I should have linked it.

It won't be restricted to JDK8, but it is only currently built/tested on JDK8. We are planning to port to new LTS JDK versions once we assess the requirements/feature support that is needed to accommodate them.

@keithc-ca
Copy link
Member

we don't need opt_snapshots; its presence here is a mistake

The code to adjust opt_snapshots is appropriate for UMA builds.

@ThanHenderson
Copy link
Contributor Author

The code to adjust opt_snapshots is appropriate for UMA builds.

I mean that it isn't used at all, nor specified, in the OpenJ9 patch, and we can add it later if we need.

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

Successfully merging this pull request may close these issues.

3 participants