Skip to content

Commit

Permalink
Fix JFR synchronization issues
Browse files Browse the repository at this point in the history 
- Assert that VM access is always held while interacting with JFR buffer
- Previously, we stored classhashtable entries. Class hashtable can grow
and these entries become stale. Instead use romClass as the key for the
package table
- Previously, we iterated ClassEntry hashtables while adding entries to
them (see fixupShallowEntries), this results in corruption. Instead we
now make a list of the classes we need to scan (findShallowEntries),
then walk the list and add scan entries after.
- Dont create tables with J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION since we
cant use that flag and iterate them.

Other fixes:
- dont assume moduleName is always non-NULL
- skip thread flush if the buffer is NULL
- add aserts to non-sensible events

Signed-off-by: Tobi Ajila <[email protected]>
  • Loading branch information
@tajila
tajila committed Nov 14, 2024
1 parent 2583a83 commit 99ca9c9
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 38 deletions.
14 changes: 8 additions & 6 deletions runtime/util/cphelp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ getClassLocation(J9VMThread * currentThread, J9Class * clazz, UDATA *length)

if (NULL != classLoader->classLocationHashTable) {
J9ClassLocation *classLocation = vmFuncs->findClassLocationForClass(currentThread, clazz);

if (NULL != classLocation) {
switch(classLocation->locationType) {
case LOAD_LOCATION_PATCH_PATH_NON_GENERATED:
Expand All @@ -86,7 +86,7 @@ getClassLocation(J9VMThread * currentThread, J9Class * clazz, UDATA *length)

case LOAD_LOCATION_CLASSPATH_NON_GENERATED:
case LOAD_LOCATION_CLASSPATH:
rc = getClassPathEntry(currentThread, classLoader, classLocation->entryIndex, &entry);
rc = getClassPathEntry(currentThread, classLoader, classLocation->entryIndex, &entry);
if (0 == rc) {
*length = entry.pathLength;
path = entry.path;
Expand Down Expand Up @@ -149,7 +149,9 @@ getModuleJRTURL(J9VMThread *currentThread, J9ClassLoader *classLoader, J9Module
if (NULL == jrtURL) {
if (J9_ARE_ALL_BITS_SET(javaVM->runtimeFlags, J9_RUNTIME_JAVA_BASE_MODULE_CREATED)) {
/* set jrt URL for the module */
jrtURL = vmFuncs->copyStringToJ9UTF8WithMemAlloc(currentThread, module->moduleName, J9_STR_NONE, "jrt:/", 5, NULL, 0);
if (NULL != module->moduleName) {
jrtURL = vmFuncs->copyStringToJ9UTF8WithMemAlloc(currentThread, module->moduleName, J9_STR_NONE, "jrt:/", 5, NULL, 0);
}

if (NULL == jrtURL) {
goto _exit;
Expand All @@ -162,7 +164,7 @@ getModuleJRTURL(J9VMThread *currentThread, J9ClassLoader *classLoader, J9Module
if (NULL == jrtURL) {
goto _exit;
}
memcpy(J9UTF8_DATA(jrtURL), J9UTF8_DATA(&jrtJavaBaseUrl), length);
memcpy(J9UTF8_DATA(jrtURL), J9UTF8_DATA(&jrtJavaBaseUrl), length);
J9UTF8_SET_LENGTH(jrtURL, length);
}
moduleInfo->jrtURL = jrtURL;
Expand Down Expand Up @@ -219,9 +221,9 @@ addJarToSystemClassLoaderClassPathEntries(J9JavaVM *vm, const char *filename)

#if defined(J9VM_OPT_SHARED_CLASSES)
if (J9_ARE_ALL_BITS_SET(classLoader->flags, J9CLASSLOADER_SHARED_CLASSES_ENABLED)) {
/*
/*
* Warm up the classpath entry so that the Classpath stored in the cache has the correct info.
* This is required because when we are finding classes in the cache, initializeClassPathEntry is not called
* This is required because when we are finding classes in the cache, initializeClassPathEntry is not called
* */
if (vm->internalVMFunctions->initializeClassPathEntry(vm, cpEntry) != CPE_TYPE_JAR) {
goto done;
Expand Down
30 changes: 20 additions & 10 deletions runtime/vm/JFRConstantPoolTypes.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ VM_JFRConstantPoolTypes::jfrPackageHashFn(void *key, void *userData)
{
PackageEntry *packageEntry = (PackageEntry *) key;

return *(UDATA*)&packageEntry->pkgID;
return *(UDATA*)&packageEntry->romClass;
}

UDATA
Expand All @@ -59,7 +59,7 @@ VM_JFRConstantPoolTypes::jfrPackageHashEqualFn(void *tableNode, void *queryNode,
PackageEntry *tableEntry = (PackageEntry *) tableNode;
PackageEntry *queryEntry = (PackageEntry *) queryNode;

return tableEntry->pkgID == queryEntry->pkgID;
return tableEntry->romClass == queryEntry->romClass;
}

UDATA
Expand Down Expand Up @@ -308,17 +308,28 @@ VM_JFRConstantPoolTypes::walkStackTraceTablePrint(void *entry, void *userData)
return FALSE;
}


UDATA
VM_JFRConstantPoolTypes::fixupShallowEntries(void *entry, void *userData)
VM_JFRConstantPoolTypes::findShallowEntries(void *entry, void *userData)
{
ClassEntry *tableEntry = (ClassEntry *) entry;
VM_JFRConstantPoolTypes *cp = (VM_JFRConstantPoolTypes*) userData;
J9Pool *shallowEntries = (J9Pool*) userData;

cp->getClassEntry(tableEntry->clazz);
ClassEntry **newEntry = (ClassEntry**)pool_newElement(shallowEntries);
*newEntry = tableEntry;

return FALSE;
}

void
VM_JFRConstantPoolTypes::fixupShallowEntries(void *entry, void *userData)
{
ClassEntry *tableEntry = *(ClassEntry **) entry;
VM_JFRConstantPoolTypes *cp = (VM_JFRConstantPoolTypes*) userData;

cp->getClassEntry(tableEntry->clazz);
}

UDATA
VM_JFRConstantPoolTypes::mergeStringUTF8EntriesToGlobalTable(void *entry, void *userData)
{
Expand All @@ -335,11 +346,9 @@ VM_JFRConstantPoolTypes::mergePackageEntriesToGlobalTable(void *entry, void *use
{
PackageEntry *tableEntry = (PackageEntry *) entry;
VM_JFRConstantPoolTypes *cp = (VM_JFRConstantPoolTypes*) userData;
UDATA packageNameLength = 0;

getPackageName(tableEntry->pkgID, &packageNameLength);
cp->_globalStringTable[tableEntry->index + cp->_stringUTF8Count] = tableEntry;
cp->_requiredBufferSize += packageNameLength;
cp->_requiredBufferSize += tableEntry->packageNameLength;
return FALSE;
}

Expand Down Expand Up @@ -472,7 +481,8 @@ VM_JFRConstantPoolTypes::addPackageEntry(J9Class *clazz)
_buildResult = OK;

pkgID = hashPkgTableAt(clazz->classLoader, clazz->romClass);
entry->pkgID = pkgID;
entry->romClass = clazz->romClass;
entry->ramClass = clazz;

if (NULL == pkgID) {
/* default pacakge */
Expand All @@ -491,7 +501,7 @@ VM_JFRConstantPoolTypes::addPackageEntry(J9Class *clazz)
entry->moduleIndex = addModuleEntry(clazz->module);
if (isResultNotOKay()) goto done;

packageName = (const char *) getPackageName(entry->pkgID, &packageNameLength);
packageName = (const char *) getPackageName(pkgID, &packageNameLength);
if (NULL == packageName) {
_buildResult = InternalVMError;
goto done;
Expand Down
45 changes: 33 additions & 12 deletions runtime/vm/JFRConstantPoolTypes.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ struct ClassEntry {
};

struct PackageEntry {
J9PackageIDTableEntry *pkgID;
J9ROMClass *romClass;
J9Class *ramClass;
U_32 moduleIndex;
BOOLEAN exported;
U_32 packageNameLength;
Expand Down Expand Up @@ -406,7 +407,9 @@ class VM_JFRConstantPoolTypes {

static UDATA walkStackTraceTablePrint(void *entry, void *userData);

static UDATA fixupShallowEntries(void *entry, void *userData);
static UDATA findShallowEntries(void *entry, void *userData);

static void fixupShallowEntries(void *anElement, void *userData);

static UDATA walkMethodTablePrint(void *entry, void *userData);

Expand Down Expand Up @@ -750,6 +753,7 @@ class VM_JFRConstantPoolTypes {
void loadEvents()
{
J9JFRBufferWalkState walkstate = {0};
J9Pool *shallowEntries = NULL;
J9JFREvent *event = jfrBufferStartDo(&_vm->jfrBuffer, &walkstate);

while (NULL != event) {
Expand Down Expand Up @@ -782,8 +786,25 @@ class VM_JFRConstantPoolTypes {
event = jfrBufferNextDo(&walkstate);
}

hashTableForEachDo(_classTable, &fixupShallowEntries, this);
if (isResultNotOKay()) {
goto done;
}

shallowEntries = pool_new(sizeof(ClassEntry**), 0, sizeof(U_64), 0, J9_GET_CALLSITE(), OMRMEM_CATEGORY_VM, POOL_FOR_PORT(privatePortLibrary));
if (NULL == shallowEntries) {
_buildResult = OutOfMemory;
goto done;
}

hashTableForEachDo(_classTable, &findShallowEntries, shallowEntries);
pool_do(shallowEntries, &fixupShallowEntries, this);

pool_kill(shallowEntries);

mergeStringTables();

done:
return;
}

U_32 consumeStackTrace(J9VMThread *walkThread, UDATA *walkStateCache, UDATA numberOfFrames) {
Expand Down Expand Up @@ -1099,55 +1120,55 @@ class VM_JFRConstantPoolTypes {
, _firstPackageEntry(NULL)
, _requiredBufferSize(0)
{
_classTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ClassEntry), sizeof(ClassEntry *), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, jfrClassHashFn, jfrClassHashEqualFn, NULL, _vm);
_classTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ClassEntry), sizeof(ClassEntry *), 0, J9MEM_CATEGORY_CLASSES, jfrClassHashFn, jfrClassHashEqualFn, NULL, _vm);
if (NULL == _classTable) {
_buildResult = OutOfMemory;
goto done;
}

_packageTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(PackageEntry), sizeof(PackageEntry *), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, jfrPackageHashFn, jfrPackageHashEqualFn, NULL, _vm);
_packageTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(PackageEntry), sizeof(PackageEntry *), 0, J9MEM_CATEGORY_CLASSES, jfrPackageHashFn, jfrPackageHashEqualFn, NULL, _vm);
if (NULL == _packageTable) {
_buildResult = OutOfMemory;
goto done;
}

_classLoaderTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ClassloaderEntry), sizeof(J9ClassLoader*), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, classloaderNameHashFn, classloaderNameHashEqualFn, NULL, _vm);
_classLoaderTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ClassloaderEntry), sizeof(J9ClassLoader*), 0, J9MEM_CATEGORY_CLASSES, classloaderNameHashFn, classloaderNameHashEqualFn, NULL, _vm);
if (NULL == _classLoaderTable) {
_buildResult = OutOfMemory;
goto done;
}

_methodTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(MethodEntry), sizeof(J9ROMMethod*), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, methodNameHashFn, methodNameHashEqualFn, NULL, _vm);
_methodTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(MethodEntry), sizeof(J9ROMMethod*), 0, J9MEM_CATEGORY_CLASSES, methodNameHashFn, methodNameHashEqualFn, NULL, _vm);
if (NULL == _methodTable) {
_buildResult = OutOfMemory;
goto done;
}

_stringUTF8Table = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(StringUTF8Entry), sizeof(StringUTF8Entry*), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, jfrStringUTF8HashFn, jfrStringUTF8HashEqualFn, NULL, _vm);
_stringUTF8Table = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(StringUTF8Entry), sizeof(StringUTF8Entry*), 0, J9MEM_CATEGORY_CLASSES, jfrStringUTF8HashFn, jfrStringUTF8HashEqualFn, NULL, _vm);
if (NULL == _stringUTF8Table) {
_buildResult = OutOfMemory;
goto done;
}

_moduleTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ModuleEntry), sizeof(ModuleEntry*), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, jfrModuleHashFn, jfrModuleHashEqualFn, NULL, _vm);
_moduleTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ModuleEntry), sizeof(ModuleEntry*), 0, J9MEM_CATEGORY_CLASSES, jfrModuleHashFn, jfrModuleHashEqualFn, NULL, _vm);
if (NULL == _moduleTable) {
_buildResult = OutOfMemory;
goto done;
}

_threadTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ThreadEntry), sizeof(U_64), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, threadHashFn, threadHashEqualFn, NULL, _currentThread);
_threadTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ThreadEntry), sizeof(U_64), 0, J9MEM_CATEGORY_CLASSES, threadHashFn, threadHashEqualFn, NULL, _currentThread);
if (NULL == _threadTable) {
_buildResult = OutOfMemory;
goto done;
}

_stackTraceTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(StackTraceEntry), sizeof(U_64), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, stackTraceHashFn, stackTraceHashEqualFn, NULL, _vm);
_stackTraceTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(StackTraceEntry), sizeof(U_64), 0, J9MEM_CATEGORY_CLASSES, stackTraceHashFn, stackTraceHashEqualFn, NULL, _vm);
if (NULL == _stackTraceTable) {
_buildResult = OutOfMemory;
goto done;
}

_threadGroupTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ThreadGroupEntry), sizeof(U_64), J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION, J9MEM_CATEGORY_CLASSES, threadGroupHashFn, threadGroupHashEqualFn, NULL, _vm);
_threadGroupTable = hashTableNew(OMRPORT_FROM_J9PORT(privatePortLibrary), J9_GET_CALLSITE(), 0, sizeof(ThreadGroupEntry), sizeof(U_64), 0, J9MEM_CATEGORY_CLASSES, threadGroupHashFn, threadGroupHashEqualFn, NULL, _vm);
if (NULL == _threadGroupTable) {
_buildResult = OutOfMemory;
goto done;
Expand Down
Loading

0 comments on commit 99ca9c9

Please sign in to comment.