Delay lock-related operations in single thread mode #14583

tajila · 2022-02-23T19:50:34Z

Delay lock-related operations in single thread mode

This PR delays notify/notifyAll operations by intercepting the
Object.notify* INLs and saving the instances and operations to a queue.
These instances must be saved as global JNI refs as we cannot save them
in localref storage and disallowing a GC is not feasible. At the end of
the single thread phase all the operations will be executed by the
checkpointing thread.

See #14584

Handles case 4) Notify

Signed-off-by: Tobi Ajila atobia@ca.ibm.com

tajila · 2022-02-23T21:35:38Z

@DanHeidinga Please review these changes

runtime/criusupport/criusupport.cpp

DanHeidinga · 2022-03-04T13:45:58Z

runtime/nls/j9cl/j9jcl.nls

@@ -515,3 +515,11 @@ J9NLS_JCL_CRIU_FAILED_TO_RUN_INTERNAL_RESTORE_HOOKS.explanation=An error occured
 J9NLS_JCL_CRIU_FAILED_TO_RUN_INTERNAL_RESTORE_HOOKS.system_action=The JVM will throw a RestoreException.
 J9NLS_JCL_CRIU_FAILED_TO_RUN_INTERNAL_RESTORE_HOOKS.user_response=View CRIU documentation to determine how to resolve the error.
 # END NON-TRANSLATABLE
+
+J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS=Could not run delayed identity operations succesfully, errno=%li


Suggested change

J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS=Could not run delayed identity operations succesfully, errno=%li

J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS=Could not run delayed lock-related operations succesfully, errno=%li

Identity makes sense in the context of Valhalla but less so here. Should we just call it lock-related? Or are there other operations as well?

I thought there would be more, but now I think it will just be wait/notify* that we handle in this manner

DanHeidinga · 2022-03-04T13:46:21Z

runtime/nls/j9cl/j9jcl.nls

+J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS=Could not run delayed identity operations succesfully, errno=%li
+# START NON-TRANSLATABLE
+J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS.sample_input_1=1
+J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS.explanation=An error occured when the JVM attempted to run delayed identity operations.


Same concern with 'identity' here

DanHeidinga · 2022-03-04T13:49:51Z

runtime/vm/BytecodeInterpreter.hpp

+		} else {
+#endif /* defined(J9VM_OPT_CRIU_SUPPORT) */
+			if (VM_ObjectMonitor::getMonitorForNotify(_currentThread, receiver, &monitorPtr, true)) {


Suggested change

} else {

#endif /* defined(J9VM_OPT_CRIU_SUPPORT) */

if (VM_ObjectMonitor::getMonitorForNotify(_currentThread, receiver, &monitorPtr, true)) {

} else

#endif /* defined(J9VM_OPT_CRIU_SUPPORT) */

{

if (VM_ObjectMonitor::getMonitorForNotify(_currentThread, receiver, &monitorPtr, true)) {

If you put the opening { outside the ifdef, you can avoid the extra ifdef to close it later. Basically, treat it like an extra (otherwise unnecessary) scope that is only an else block for the ifdef case

DanHeidinga · 2022-03-04T13:53:04Z

runtime/vm/CRIUHelpers.cpp

 #define JAVA_UTIL_RANDOM "java/util/Random"
-		J9Class *juRandomClass = hashClassTableAt(vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));
+	juRandomClass = hashClassTableAt(vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));


This needs to be protected by the classTableMutex or use the peekClassHashTable function instead

runtime/vm/CRIUHelpers.cpp

DanHeidinga · 2022-03-04T14:01:45Z

runtime/vm/CRIUHelpers.cpp

+		}
+		if (!VM_ObjectMonitor::getMonitorForNotify(currentThread, instance, &monitorPtr, true)) {
+			if (NULL != monitorPtr) {
+				/* another thread owns the lock, shouldn't be possible */


DanHeidinga · 2022-03-04T14:02:51Z

runtime/vm/CRIUHelpers.cpp

+	J9Pool *delayedSingleThreadModeOpRecords = vm->checkpointState.delayedSingleThreadModeOpRecord;
+	J9InternalVMFunctions* vmFuncs = vm->internalVMFunctions;
+	J9DelayedSingleThreadModeOpRecord *delayedSingleThreadModeOp = (J9DelayedSingleThreadModeOpRecord*) pool_startDo(delayedSingleThreadModeOpRecords, &walkState);
+	omrthread_monitor_t monitorPtr = NULL;


Can you declare this in the loop to shorten it's lifetime?

DanHeidinga · 2022-03-04T14:10:22Z

runtime/vm/CRIUHelpers.cpp

+		}
+
+		monitorPtr = NULL;
+		vmFuncs->j9jni_deleteGlobalRef((JNIEnv*) currentThread, delayedSingleThreadModeOp->globalObjectRef, JNI_FALSE);


Each item should be removed from the pool after the its been notified - use pool_removeElement which says it is safe to use during an iteration

DanHeidinga · 2022-03-04T14:14:58Z

runtime/oti/j9nonbuilder.h

@@ -4070,11 +4070,20 @@ typedef struct J9InternalHookRecord {
 	struct J9Pool *instanceObjects;
 } J9InternalHookRecord;

+typedef struct J9DelayedSingleThreadModeOpRecord {
+	jobject globalObjectRef;
+	UDATA operation;


Does pool iteration guarantee the same order FIFO iteration order? i.e. the first notify we delay should be the first we do after exiting single threaded mode. Similarly the 2nd should be the 2nd, etc.

If the pool iteration order isn't guaranteed (and documented!) I think these should have a J9DelayedSingleThreadModeOpRecord *next field as well and they should be stored in a linked list. The pool is fine for backing the allocation but we should iterate through the list to do the notifies.

And likely need two pointers - one for the head of the list and one for the tail so we can insert quickly at the tail and iterate from the head

tajila · 2022-03-08T21:38:22Z

@DanHeidinga Changes are ready for another look

tajila · 2022-03-09T23:20:30Z

@DanHeidinga

I fixed a small issue with not releasing locks when there are no waiters and added more tracepoints. Changes are ready for another look

DanHeidinga · 2022-03-11T18:54:35Z

runtime/vm/CRIUHelpers.cpp

+
+		vmFuncs->j9jni_deleteGlobalRef((JNIEnv*) currentThread, delayedLockingOperation->globalObjectRef, JNI_FALSE);
+		pool_removeElement(vm->checkpointState.delayedLockingOperationsRecords, delayedLockingOperation);
+		delayedLockingOperation = J9_LINKED_LIST_NEXT_DO(vm->checkpointState.delayedLockingOperationsRoot, delayedLockingOperation);


This is a use after free issue as you've removed the element from the pool (free) and then dereferenced it.

DanHeidinga

Some minor formatting issues to shrink variable lifetimes and use c++ style casts, otherwise fine

DanHeidinga · 2022-03-15T20:54:38Z

runtime/vm/CRIUHelpers.cpp

+		}
+
+		vmFuncs->j9jni_deleteGlobalRef((JNIEnv*) currentThread, delayedLockingOperation->globalObjectRef, JNI_FALSE);
+		lastOperation = delayedLockingOperation;


Given this is a cpp file, can we declare lastOperation here rather than at the top of the scope? It makes it easer to reason about the lifetime of the variable

Suggested change

lastOperation = delayedLockingOperation;

J9DelayedLockingOpertionsRecord *lastOperation = delayedLockingOperation;

DanHeidinga · 2022-03-15T20:56:24Z

runtime/vm/CRIUHelpers.hpp

+			goto throwOOM;
+		}
+
+		newRecord = (J9DelayedLockingOpertionsRecord*) pool_newElement(vm->checkpointState.delayedLockingOperationsRecords);


Suggested change

newRecord = (J9DelayedLockingOpertionsRecord*) pool_newElement(vm->checkpointState.delayedLockingOperationsRecords);

J9DelayedLockingOpertionsRecord *newRecord = static_cast<J9DelayedLockingOpertionsRecord*>( pool_newElement(vm->checkpointState.delayedLockingOperationsRecords));

Can't move this one as well because of the goto's

DanHeidinga · 2022-03-15T20:57:30Z

runtime/vm/CRIUHelpers.cpp

+{
+	J9JavaVM *vm = currentThread->javaVM;
+	J9InternalVMFunctions* vmFuncs = vm->internalVMFunctions;
+	J9DelayedLockingOpertionsRecord *delayedLockingOperation = (J9DelayedLockingOpertionsRecord*) J9_LINKED_LIST_START_DO(vm->checkpointState.delayedLockingOperationsRoot);


Suggested change

J9DelayedLockingOpertionsRecord *delayedLockingOperation = (J9DelayedLockingOpertionsRecord*) J9_LINKED_LIST_START_DO(vm->checkpointState.delayedLockingOperationsRoot);

J9DelayedLockingOpertionsRecord *delayedLockingOperation = static_cast<J9DelayedLockingOpertionsRecord*>(J9_LINKED_LIST_START_DO(vm->checkpointState.delayedLockingOperationsRoot));

DanHeidinga · 2022-03-15T20:58:20Z

runtime/vm/CRIUHelpers.cpp

 #define JAVA_UTIL_RANDOM "java/util/Random"
-		J9Class *juRandomClass = hashClassTableAt(vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));
+	juRandomClass = peekClassHashTable(currentThread, vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));


Suggested change

juRandomClass = peekClassHashTable(currentThread, vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));

J9Class *juRandomClass = peekClassHashTable(currentThread, vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));

I cant move this one because of the jump to done:

It might require an additional scope so the change becomes:

{ J9Class *juRandomClass = peekClassHashTable(currentThread, vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM)); .... if (NULL != juRandomClass) { addInternalJVMCheckpointHook(currentThread, TRUE, juRandomClass, FALSE, juRandomReseed); } }

But I'm OK if you want to leave it as is. The method's clear enough that this isn't critical

tajila · 2022-03-17T14:46:36Z

@DanHeidinga The PR is ready for another look

DanHeidinga · 2022-03-17T15:59:42Z

@tajila do we have CRIU-enabled PR builds? If so, do they need a special trigger phrase?

tajila · 2022-03-17T19:56:41Z

@DanHeidinga

I've made the changes

@tajila do we have CRIU-enabled PR builds? If so, do they need a special trigger phrase?

No, unfortunately we dont have machine support yet

tajila · 2022-03-17T19:56:56Z

Jenkins test sanity win jdk11

DanHeidinga · 2022-03-18T13:48:28Z

The windows PR build is failing due to code reaching for vm->checkpointState which is only defined under #if defined(J9VM_OPT_CRIU_SUPPORT).

tajila · 2022-03-18T17:16:25Z

Jenkins test sanity win jdk11

This PR delays notify/notifyAll operations by intercepting the Object.notify* INLs and saving the instances and operations to a queue. These instances must be saved as global JNI refs as we cannot save them in localref storage and disallowing a GC is not feasible. At the end of the single thread phase all the operations will be executed by the checkpointing thread. See eclipse-openj9#14584 Handles case 4) Notify Signed-off-by: Tobi Ajila <atobia@ca.ibm.com>

tajila · 2022-03-18T17:20:26Z

Jenkins test sanity win jdk11

tajila · 2022-03-19T17:38:17Z

Jenkins test sanity win jdk11

DanHeidinga · 2022-03-21T12:55:22Z

Jenkins test sanity aix jdk8

DanHeidinga · 2022-03-21T12:55:53Z

Jenkins test sanity xlinux jdk17

tajila force-pushed the criu branch 5 times, most recently from 981f821 to 68dcc4e Compare February 23, 2022 20:42

tajila marked this pull request as ready for review February 23, 2022 21:35

tajila requested a review from DanHeidinga February 23, 2022 21:35

tajila force-pushed the criu branch 5 times, most recently from 91599b5 to 9adeb6b Compare March 3, 2022 16:29

DanHeidinga reviewed Mar 4, 2022

View reviewed changes

JasonFengJ9 mentioned this pull request Mar 4, 2022

Prevent contended monitor enters at CRIU single thread mode #14651

Merged

tajila force-pushed the criu branch 12 times, most recently from 8a6729e to b61b9cd Compare March 8, 2022 20:54

tajila force-pushed the criu branch 2 times, most recently from 2e72dac to d375fd8 Compare March 9, 2022 15:16

tajila force-pushed the criu branch 2 times, most recently from 66b4528 to 8eaadf6 Compare March 9, 2022 21:00

DanHeidinga reviewed Mar 11, 2022

View reviewed changes

tajila force-pushed the criu branch from 8eaadf6 to 2c8765d Compare March 13, 2022 20:08

tajila modified the milestone: Backlog Mar 13, 2022

DanHeidinga reviewed Mar 15, 2022

View reviewed changes

tajila mentioned this pull request Mar 16, 2022

CRIU support: add checkpoint/restore environment variables ibmruntimes/openj9-openjdk-jdk11#486

Merged

tajila force-pushed the criu branch 2 times, most recently from 3f85328 to c66c635 Compare March 17, 2022 14:30

tajila force-pushed the criu branch from c66c635 to 1423616 Compare March 17, 2022 19:47

DanHeidinga self-assigned this Mar 18, 2022

tajila force-pushed the criu branch from 1423616 to b1f670d Compare March 18, 2022 17:16

tajila force-pushed the criu branch from b1f670d to 4fd47d9 Compare March 18, 2022 17:17

DanHeidinga approved these changes Mar 21, 2022

View reviewed changes

DanHeidinga merged commit cc586f0 into eclipse-openj9:master Mar 21, 2022

JasonFengJ9 added comp:vm criu Used to track CRIU snapshot related work labels Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay lock-related operations in single thread mode #14583

Delay lock-related operations in single thread mode #14583

tajila commented Feb 23, 2022 •

edited

tajila commented Feb 23, 2022

DanHeidinga Mar 4, 2022

tajila Mar 7, 2022

DanHeidinga Mar 4, 2022

DanHeidinga Mar 4, 2022

DanHeidinga Mar 4, 2022

DanHeidinga Mar 4, 2022

DanHeidinga Mar 4, 2022

DanHeidinga Mar 4, 2022

DanHeidinga Mar 4, 2022

tajila commented Mar 8, 2022

tajila commented Mar 9, 2022

DanHeidinga Mar 11, 2022

DanHeidinga left a comment

DanHeidinga Mar 15, 2022

DanHeidinga Mar 15, 2022

tajila Mar 17, 2022

DanHeidinga Mar 15, 2022

DanHeidinga Mar 15, 2022

tajila Mar 17, 2022

DanHeidinga Mar 17, 2022

DanHeidinga Mar 17, 2022

tajila commented Mar 17, 2022

DanHeidinga commented Mar 17, 2022 •

edited

tajila commented Mar 17, 2022

tajila commented Mar 17, 2022

DanHeidinga commented Mar 18, 2022

tajila commented Mar 18, 2022

tajila commented Mar 18, 2022

tajila commented Mar 19, 2022

DanHeidinga commented Mar 21, 2022

DanHeidinga commented Mar 21, 2022

	J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS=Could not run delayed identity operations succesfully, errno=%li
	J9NLS_JCL_CRIU_FAILED_DELAY_INDENTIY_OPS=Could not run delayed lock-related operations succesfully, errno=%li

	lastOperation = delayedLockingOperation;
	J9DelayedLockingOpertionsRecord *lastOperation = delayedLockingOperation;

	newRecord = (J9DelayedLockingOpertionsRecord*) pool_newElement(vm->checkpointState.delayedLockingOperationsRecords);
	J9DelayedLockingOpertionsRecord newRecord = static_cast<J9DelayedLockingOpertionsRecord>( pool_newElement(vm->checkpointState.delayedLockingOperationsRecords));

	J9DelayedLockingOpertionsRecord delayedLockingOperation = (J9DelayedLockingOpertionsRecord) J9_LINKED_LIST_START_DO(vm->checkpointState.delayedLockingOperationsRoot);
	J9DelayedLockingOpertionsRecord delayedLockingOperation = static_cast<J9DelayedLockingOpertionsRecord>(J9_LINKED_LIST_START_DO(vm->checkpointState.delayedLockingOperationsRoot));

	juRandomClass = peekClassHashTable(currentThread, vm->systemClassLoader, (U_8 *)JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));
	J9Class juRandomClass = peekClassHashTable(currentThread, vm->systemClassLoader, (U_8 )JAVA_UTIL_RANDOM, LITERAL_STRLEN(JAVA_UTIL_RANDOM));

Delay lock-related operations in single thread mode #14583

Delay lock-related operations in single thread mode #14583

Conversation

tajila commented Feb 23, 2022 • edited

tajila commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tajila commented Mar 8, 2022

tajila commented Mar 9, 2022

Choose a reason for hiding this comment

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tajila commented Mar 17, 2022

DanHeidinga commented Mar 17, 2022 • edited

tajila commented Mar 17, 2022

tajila commented Mar 17, 2022

DanHeidinga commented Mar 18, 2022

tajila commented Mar 18, 2022

tajila commented Mar 18, 2022

tajila commented Mar 19, 2022

DanHeidinga commented Mar 21, 2022

DanHeidinga commented Mar 21, 2022

tajila commented Feb 23, 2022 •

edited

DanHeidinga commented Mar 17, 2022 •

edited