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

Introduce JVMPortableRestoreMode #18252

Merged
merged 1 commit into from Oct 10, 2023
Merged

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Oct 6, 2023

Previously, the CRIURestoreNonPortableMode was used to determine if a checkpoint could be taken after restore. If CRIURestoreNonPortableMode was enabled (default mode) only a single checkpoint is allowed and the JVM doesn't need to generate portable code upon restore. This capability also allows the JVM to use standard security providers since there is no risk for that state to be serialized.

There is a need to separate out the portability capabilites and the behavioural changes that CRIURestoreNonPortableMode offers. There are cases where one wants portability as well as standard security capabilites for debugging purposes.

This PR separates out the portability capabilites and the behavioural aspects by providing an option that forces portability upon restore regardless of what CRIURestoreNonPortableMode is set to.

In the future we can add another option that forces portability in non-CRIU mode, it its desired.

@tajila tajila requested a review from dsouzai October 6, 2023 15:47
@tajila
Copy link
Contributor Author

tajila commented Oct 6, 2023

@dsouzai is there a way to turn on JIT logging to confirm it is generating portable JIT code?

@dsouzai
Copy link
Contributor

dsouzai commented Oct 6, 2023

I don't see any logging. What you could do is something like this:

diff --git a/runtime/compiler/control/HookedByTheJit.cpp b/runtime/compiler/control/HookedByTheJit.cpp
index 22dfb172c..79bef5591 100644
--- a/runtime/compiler/control/HookedByTheJit.cpp
+++ b/runtime/compiler/control/HookedByTheJit.cpp
@@ -1862,6 +1862,12 @@ static void jitHookPrepareRestore(J9HookInterface * * hookInterface, UDATA event

       /* Reinitialize the (technically unused) targetProcesssorInfo on x86 to prevent asserts */
       TR::Compiler->target.cpu.initializeTargetProcessorInfo(true);
+
+      printf("Will no longer generate portable code\n");
+      }
+   else
+      {
+      printf("Will continue to generate portable code\n");
       }

    TR::CompilationInfo * compInfo = TR::CompilationInfo::get(jitConfig);
diff --git a/runtime/compiler/control/rossa.cpp b/runtime/compiler/control/rossa.cpp
index 835fbde10..8a104f1e3 100644
--- a/runtime/compiler/control/rossa.cpp
+++ b/runtime/compiler/control/rossa.cpp
@@ -2005,6 +2005,8 @@ aboutToBootstrap(J9JavaVM * javaVM, J9JITConfig * jitConfig)
       if (!J9_ARE_ANY_BITS_SET(javaVM->extendedRuntimeFlags2, J9_EXTENDED_RUNTIME2_ENABLE_PORTABLE_SHARED_CACHE))
          TR::Compiler->relocatableTarget.cpu = TR::CPU::detectRelocatable(TR::Compiler->omrPortLib);
       jitConfig->targetProcessor = TR::Compiler->target.cpu.getProcessorDescription();
+
+      printf("Will generate portable code");
       }

@tajila tajila force-pushed the criu_3 branch 2 times, most recently from 808cf78 to 6d36d29 Compare October 6, 2023 16:11
@dsouzai
Copy link
Contributor

dsouzai commented Oct 6, 2023

The definition of isJVMInPortableRestoreMode is

BOOLEAN
isJVMInPortableRestoreMode(J9VMThread *currentThread)
{
	return !isNonPortableRestoreMode(currentThread) || J9_ARE_ALL_BITS_SET(currentThread->javaVM->checkpointState.flags, J9VM_CRIU_IS_PORTABLE_JVM_RESTORE_MODE);
}

which means that it'll return true if J9VM_CRIU_IS_NON_PORTABLE_RESTORE_MODE is not set. However, this query doesn't check whether CRIU Support is enabled. So, in all the places where isCheckpointAllowed is replaced by isJVMInPortableRestoreMode, aren't we going to execute that code even when CRIU is disabled?

@tajila tajila force-pushed the criu_3 branch 4 times, most recently from 4659938 to 92cb024 Compare October 6, 2023 17:18
@tajila
Copy link
Contributor Author

tajila commented Oct 6, 2023

So, in all the places where isCheckpointAllowed is replaced by isJVMInPortableRestoreMode, aren't we going to execute that code even when CRIU is disabled?

Ya, Ive corrected that.

Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

Overall LGTM; just requesting a couple of changes.

runtime/vm/jvminit.c Outdated Show resolved Hide resolved
runtime/compiler/control/JITClientCompilationThread.cpp Outdated Show resolved Hide resolved
@tajila tajila force-pushed the criu_3 branch 2 times, most recently from 49b6ae5 to d6fc5fb Compare October 6, 2023 18:47
@dsouzai
Copy link
Contributor

dsouzai commented Oct 6, 2023

Oh! I just recalled, you also need to change the MINOR_NUMBER in CommunicationStream.hpp since a new front end query is being added.

runtime/vm/jvminit.c Outdated Show resolved Hide resolved
@tajila tajila force-pushed the criu_3 branch 7 times, most recently from 9eb7071 to e3f09e4 Compare October 6, 2023 19:43
@tajila
Copy link
Contributor Author

tajila commented Oct 6, 2023

Ready for another look

Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

Minor changes requested.

runtime/compiler/net/MessageTypes.cpp Outdated Show resolved Hide resolved
runtime/compiler/net/MessageTypes.hpp Outdated Show resolved Hide resolved
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
Previously, the CRIURestoreNonPortableMode was used to determine if a
checkpoint could be taken after restore. If CRIURestoreNonPortableMode
was enabled (default mode) only a single checkpoint is allowed and the
JVM doesn't need to generate portable code upon restore. This capability
also allows the JVM to use standard security providers since there is no
risk for that state to be serialized.

There is a need to separate out the portability capabilites and the
behavioural changes that CRIURestoreNonPortableMode offers. There are
cases where one wants portability as well as standard security
capabilites for debugging purposes.

This PR separates out the portability capabilites and the behavioural
aspects by providing an option that forces portability upon restore
regardless of what CRIURestoreNonPortableMode is set to.

In the future we can add another option that forces portability in
non-CRIU mode, it its desired.

Signed-off-by: Tobi Ajila <atobia@ca.ibm.com>
@tajila
Copy link
Contributor Author

tajila commented Oct 6, 2023

Ready for another look

@dsouzai
Copy link
Contributor

dsouzai commented Oct 6, 2023

jenkins test sanity all jdk17,jdk21

@dsouzai dsouzai self-assigned this Oct 6, 2023
@dsouzai dsouzai added comp:vm comp:jit criu Used to track CRIU snapshot related work labels Oct 6, 2023
@tajila
Copy link
Contributor Author

tajila commented Oct 10, 2023

All test passed in the windows run, just an infra issue: https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_sanity.functional_x86-64_windows_Personal/315/tapResults/.

testServerComesUpAfterClient, testServerGoesDownAnotherComesUp, testServerUnreachableForAWhile failed on PPC (17 and 21), known issue #14594

@dsouzai dsouzai merged commit 5570f22 into eclipse-openj9:master Oct 10, 2023
31 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit comp:vm criu Used to track CRIU snapshot related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants