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 stronger guarantees to System.gc() #18044

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Aug 30, 2023

The JCL code expects that when System.GC() is called a GC occurs and finalizable objects are finalized. Historically, J9 has never provided this guarantee. We have often had to modify tests to ensure a GC is triggered at the appropriate time. Increasingly, we are finding that more and more tests rely on this behaviour, and most importantly, core JCL code now relies on this behaviour. This PR ensures that a call to System.gc() triggers a GC and runs finalization.

@tajila
Copy link
Contributor Author

tajila commented Aug 30, 2023

jenkins test sanity alinux64 jdk21

@tajila
Copy link
Contributor Author

tajila commented Aug 30, 2023

jenkins test sanity alinux64 jdk21

@gacholio
Copy link
Contributor

Finalization has been deprecated for many releases - it seems odd that the JCL would rely on unspecified and completely unreliable behaviour.

@tajila
Copy link
Contributor Author

tajila commented Aug 30, 2023

jenkins compile win jdk8

@tajila
Copy link
Contributor Author

tajila commented Aug 30, 2023

@gacholio I had to add an explicit internalReleaseVMAccess because finalization cannot be run when VMAccess is held

@tajila
Copy link
Contributor Author

tajila commented Aug 30, 2023

jenkins test sanity alinux64 jdk21

1 similar comment
@tajila
Copy link
Contributor Author

tajila commented Aug 30, 2023

jenkins test sanity alinux64 jdk21

Copy link
Contributor

@gacholio gacholio left a comment

Choose a reason for hiding this comment

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

This is no longer valid. You will need to reacquire VM access and exit the VM properly.

@gacholio
Copy link
Contributor

Java_java_lang_ref_Finalizer_runFinalizationImpl appears to make the same incorrect assumption about VM access and enter/exit - consider fixing this here as well.

@gacholio
Copy link
Contributor

Also note the ifdefs in the native - those should probably be mirrored in the JVM call.

@gacholio
Copy link
Contributor

gacholio commented Aug 30, 2023

I suggest

JNIEXPORT void JNICALL
JVM_GC_Impl(void)
{
	J9VMThread *currentThread = VM.javaVM->internalVMFunctions->currentVMThread(VM.javaVM);
	J9InternalVMFunctions *vmFuncs = VM.javaVM->internalVMFunctions;
	J9VMThread *currentThread = vmFuncs->currentVMThread(VM.javaVM);
	J9MemoryManagerFunctions *mmFuncs = VM.javaVM->memoryManagerFunctions;

	Trc_SunVMI_GC_Entry(currentThread);

#if defined(J9VM_INTERP_ATOMIC_FREE_JNI)
	vmFuncs->internalEnterVMFromJNI(currentThread);
#else /* J9VM_INTERP_ATOMIC_FREE_JNI */
	vmFuncs->internalAcquireVMAccess(currentThread);
#endif /* J9VM_INTERP_ATOMIC_FREE_JNI */

	mmFuncs->j9gc_modron_global_collect(currentThread);
	mmFuncs->j9gc_modron_global_collect(currentThread);

	/* Release VMAccess as finalization cannot be run with it */
	vmFuncs->internalReleaseVMAccess(currentThread);
	mmFuncs->runFinalization(currentThread);

#if defined(J9VM_INTERP_ATOMIC_FREE_JNI)
	vmFuncs->internalAcquireVMAccess(currentThread);
	vmFuncs->internalExitVMToJNI(currentThread);
#endif /* J9VM_INTERP_ATOMIC_FREE_JNI */

	Trc_SunVMI_GC_Exit(currentThread);
}

and

void JNICALL
Java_java_lang_ref_Finalizer_runFinalizationImpl(JNIEnv *env, jclass recv)
{
	J9VMThread *currentThread = (J9VMThread*)env;
	J9JavaVM *vm = currentThread->javaVM;
	J9MemoryManagerFunctions *mmFuncs = vm->memoryManagerFunctions;
#if defined(J9VM_INTERP_ATOMIC_FREE_JNI)
	J9InternalVMFunctions *vmFuncs = vm->internalVMFunctions;
	vmFuncs->internalEnterVMFromJNI(currentThread);
	vmFuncs->internalReleaseVMAccess(currentThread);
#endif /* J9VM_INTERP_ATOMIC_FREE_JNI */
	mmFuncs->runFinalization(currentThread);
#if defined(J9VM_INTERP_ATOMIC_FREE_JNI)
	vmFuncs->internalAcquireVMAccess(currentThread);
	vmFuncs->internalExitVMToJNI(currentThread);
#endif /* J9VM_INTERP_ATOMIC_FREE_JNI */
}

@gacholio
Copy link
Contributor

gacholio commented Aug 30, 2023

Also note that this doesn't really fix anything - the finalizers themselves run on another thread, so there's no guarantee that any finalization has taken place when the JVM function / native method returns.

@dmitripivkine Please correct me if runFinalization does in fact ensure that finalizers have completed before returning.

@dmitripivkine
Copy link
Contributor

Also note that this doesn't really fix anything - the finalizers themselves run on another thread, so there's no guarantee that any finalization has taken place when the JVM function / native method returns.

@dmitripivkine Please correct me if runFinalization does in fact ensure that finalizers have completed before returning.

The idea of runFinalization is to force Finalizer threads run non-stop until queue is empty.

@gacholio
Copy link
Contributor

The idea of runFinalization is to force Finalizer threads run non-stop until queue is empty.

Does this actually mean that all existing finalizers, and any that are created during the call will be processed? The queue would technically be empty while the finalizer was processing the last object in the queue.

@dmitripivkine
Copy link
Contributor

The idea of runFinalization is to force Finalizer threads run non-stop until queue is empty.

Does this actually mean that all existing finalizers, and any that are created during the call will be processed? The queue would technically be empty while the finalizer was processing the last object in the queue.

I see your point, I will check.

The JCL code expects that when System.GC() is called a GC occurs and
finalizable objects are finalized. Historically, J9 has never provided
this guarantee. We have often had to modify tests to ensure a GC is
triggered at the appropriate time. Increasingly, we are finding that
more and more tests rely on this behaviour, and most importantly, core
JCL code now relies on this behaviour. This PR ensures that a call to
System.gc() triggers a GC and runs finalization.

Signed-off-by: tajila <[email protected]>
@tajila
Copy link
Contributor Author

tajila commented Aug 30, 2023

jenkins test sanity alinux64 jdk21

Copy link
Contributor

@gacholio gacholio left a comment

Choose a reason for hiding this comment

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

Regardless of Dmitri's investigation, this is the best we can do (if there are any edge cases they'll be in the GC code). The VM access changes are not strictly necessary, in that atomic-free is currently tolerant of mistakes, but I'd rather see the right thing done, particularly in these cases where the guts of the work are potentially large.

@tajila
Copy link
Contributor Author

tajila commented Aug 31, 2023

The failure was

22:56:31.330 0x11200j9vmutil(j9vm).15     *   ** ASSERTION FAILED ** at /home/jenkins/workspace/Build_JDK21_aarch64_linux_Personal/openj9/runtime/util/mthutil.c:600: ((((uintptr_t)-1) != methodIndex))

I believe this is a known issue

@tajila
Copy link
Contributor Author

tajila commented Aug 31, 2023

jenkins test sanity alinux64 jdk21

@gacholio gacholio merged commit ecc612b into eclipse-openj9:master Aug 31, 2023
Comment on lines +627 to +628
mmFuncs->j9gc_modron_global_collect(currentThread);
mmFuncs->j9gc_modron_global_collect(currentThread);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a comment is warranted here explaining why j9gc_modron_global_collect() is run twice?

@pshipton
Copy link
Member

pshipton commented Sep 5, 2023

Created a doc issue eclipse-openj9/openj9-docs#1161

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.

6 participants