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 J9Modules and J9Packages to the VM snapshot #20830

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThanHenderson
Copy link
Contributor

Related: #20612

Signed-off-by: Nathan Henderson [email protected]

@ThanHenderson
Copy link
Contributor Author

ThanHenderson commented Dec 12, 2024

@babsingh Here is the rest of the preliminary modularity support for the VM snapshot. There are a few areas (in particular) that need improvement (the IS_RESTORE_RUN block in java11vmi.c::JVM_DefineModule and the new code in VMSnapshotImpl.cpp::removeUnpersistedClassLoaders)

I've tested on Java 11, and it works (can create a snapshot and restore) provided it is run with -Xgcpolicy:nogc (its functionality also depends on #20828). And I'll open an issue for the GC assertion failures I am seeing.

Comment on lines +371 to +490
UDATA packageNameJ9UTF8Size = packageNameLength + sizeof(J9UTF8) + 1; /* +1 for null-terminator. */
#if defined(J9VM_OPT_SNAPSHOTS)
if (IS_SNAPSHOTTING_ENABLED(vm)) {
packageName = (J9UTF8 *)vmsnapshot_allocate_memory(packageNameJ9UTF8Size, OMRMEM_CATEGORY_VM);
} else
#endif /* defined(J9VM_OPT_SNAPSHOTS) */
{
packageName = (J9UTF8 *)j9mem_allocate_memory(packageNameJ9UTF8Size, OMRMEM_CATEGORY_VM);
}
if (NULL == packageName) {
vmFuncs->setNativeOutOfMemoryError(currentThread, 0, 0);
return retval;
}
memcpy(J9UTF8_DATA(packageName), (void *)package, packageNameLength);
J9UTF8_DATA(packageName)[packageNameLength] = '\0';
J9UTF8_SET_LENGTH(packageName, (U_16)packageNameLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use addUTFNameToPackage over here? We can rewrite addUTFNameToPackage to return packageName, and J9Package->packageName can be set outside the scope this method wherever needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main other use of addUTFNameToPackage is for setting up queries to the package hash tables. Those names do not need to be persisted (nor properly free'd when freeing a J9Package) and thus they shouldn't be allocated with the vmsnapshot_allocate_memory when snapshotting is enabled.

Given that it is only on creation where J9Package::packageName needs to potentially be allocated in such a way that it can be persisted, and that is only done once in the code base, I figured that an inline version, like implemented here, was more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's keep it as-is for now. My initial intent was to aim for a common allocation path to improve maintainability and code clarity, and rely upon the compiler to do the optimizations.

runtime/j9vm/java11vmi.c Show resolved Hide resolved
runtime/j9vm/java11vmi.c Show resolved Hide resolved
runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
runtime/oti/util_api.h Outdated Show resolved Hide resolved
runtime/vm/VMSnapshotImpl.cpp Outdated Show resolved Hide resolved
runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
runtime/vm/classsupport.c Outdated Show resolved Hide resolved
runtime/vm/classsupport.c Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

jenkins test sanity.functional amac jdk21

@babsingh
Copy link
Contributor

jenkins test sanity.functional alinux jdk11

@babsingh
Copy link
Contributor

jenkins compile win jdk17

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

Successfully merging this pull request may close these issues.

2 participants