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.

Sign up for GitHub

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

Jump to bottom

Cache rom to ram mappings for exception backtrace #20169

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Cache rom to ram mappings for exception backtrace #20169

merged 1 commit into from
Sep 19, 2024

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Sep 16, 2024 •
edited
Loading

Create a hash table and access mutex to cache romClass to ramClass mappings from
exceptiondescribe.c:findJ9ClassForROMClass. This saves time iterating over multiple
class loaders to find a ram class. Entries will be deleted when a class
is unloaded or redefined.

hangshao0
hangshao0 reviewed Sep 16, 2024
View reviewed changes
runtime/vm/exceptiondescribe.c Outdated Show resolved Hide resolved
runtime/vm/exceptiondescribe.c Outdated Show resolved Hide resolved
runtime/vm/exceptiondescribe.c Outdated Show resolved Hide resolved
runtime/vm/exceptiondescribe.c Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
@hangshao0
Copy link
Contributor

A function to delete entry from the hashtable needs to be added. That function needs to be registered with J9HOOK_VM_CLASSES_UNLOAD, so that the entry can be removed from the hashtable when the J9Class is unloaded.

hangshao0
hangshao0 reviewed Sep 17, 2024
View reviewed changes
runtime/shared_common/shrinit.cpp Outdated
@@ -3509,6 +3525,19 @@ j9shr_init(J9JavaVM *vm, UDATA loadFlags, UDATA* nonfatal)
config->jclURLCache = NULL;
config->jclURLHashTable = NULL;
config->jclUTF8HashTable = NULL;
config->romToRamHashTable = hashTableNew(OMRPORT_FROM_J9PORT(vm->portLibrary),
Copy link
Contributor

Choose a reason for hiding this comment

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

The hashtable and mutext should be created only when J9_ARE_ALL_BITS_SET(config->runtimeFlags, J9SHR_RUNTIMEFLAG_ENABLE_CACHE_NON_BOOT_CLASSES) is TRUE.

Also it is better to create the mutex before the hashtable. Access to the mutex and hashtable should be protected by if (NULL != config->romToRamHashTable).

@theresa-m theresa-m force-pushed the findclass_cache branch 2 times, most recently from 11d5e41 to a3a5d30 Compare September 17, 2024 15:07
hangshao0
hangshao0 reviewed Sep 17, 2024
View reviewed changes
runtime/vm/exceptiondescribe.c Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
runtime/vm/exceptiondescribe.c Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
hangshao0
hangshao0 reviewed Sep 17, 2024
View reviewed changes
runtime/shared_common/shrinit.cpp Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
runtime/shared_common/shrinit.cpp Outdated Show resolved Hide resolved
@hangshao0
Copy link
Contributor

hangshao0 commented Sep 17, 2024 •
edited
Loading

After addressing the review comments, please create personal builds on platforms where the regression are identified and sent them to the perf team to test.

@theresa-m theresa-m force-pushed the findclass_cache branch 3 times, most recently from 1c98baa to 40619ae Compare September 17, 2024 19:47
@hangshao0
Copy link
Contributor

When the class is redefined/retransformed, the obsolete J9Class should be removed from the romToRamHashTable here (no need to add the new class into romToRamHashTable):

openj9/runtime/util/hshelp.c

Lines 2642 to 2644 in 17a3fe8

if (!fastHCR) {
vmFuncs->hashClassTableDelete(classLoader, J9UTF8_DATA(className), J9UTF8_LENGTH(className));
}

@theresa-m theresa-m force-pushed the findclass_cache branch 3 times, most recently from 042d776 to 6035c9c Compare September 18, 2024 15:07
hangshao0
hangshao0 reviewed Sep 18, 2024
View reviewed changes
runtime/util/hshelp.c Outdated Show resolved Hide resolved
@theresa-m theresa-m force-pushed the findclass_cache branch 2 times, most recently from e95e9dd to 7ada641 Compare September 18, 2024 15:44
@hangshao0
Copy link
Contributor

@theresa-m Could you update the commit message to include some more detail of the change.

@theresa-m
Cache rom to ram mappings for exception backtrace
f41b139 
Create a hash table and access mutex to cache romClass to ramClass mappings from
exceptiondescribe.c:findJ9ClassForROMClass. This saves time iterating over multiple
class loaders to find a ram class. Entries will be deleted when a class
is unloaded or redefined.

Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Copy link
Contributor Author

@theresa-m Could you update the commit message to include some more detail of the change.

I added a few more details to the description.

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended amac jdk21

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended zlinux jdk11

@hangshao0
Copy link
Contributor

Jenkins test sanity,extended alinux jdk17

@theresa-m
Copy link
Contributor Author

theresa-m commented Sep 19, 2024 •
edited
Loading

The failure is not isolated to this build. The four most recent test jobs are all failing in the same way https://openj9-jenkins.osuosl.org/job/Test_openjdk11_j9_sanity.functional_s390x_linux_Personal_testList_0/

The failure looks to be related to #20150

@hangshao0 hangshao0 merged commit 308fd4d into eclipse-openj9:master Sep 19, 2024
11 of 12 checks passed
@hangshao0
Copy link
Contributor

Please create a PR to back port this change to 0.48.

@theresa-m theresa-m mentioned this pull request Sep 19, 2024
Merged
@theresa-m
Copy link
Contributor Author

0.48 port is here #20195

keithc-ca
keithc-ca reviewed Sep 19, 2024
View reviewed changes
runtime/util/hshelp.c
@@ -2641,6 +2642,13 @@ recreateRAMClasses(J9VMThread * currentThread, J9HashTable * classHashTable, J9H
/* Delete original class from defining loader's class table */
if (!fastHCR) {
vmFuncs->hashClassTableDelete(classLoader, J9UTF8_DATA(className), J9UTF8_LENGTH(className));
if ((NULL != vm->sharedClassConfig) && (NULL != vm->sharedClassConfig->romToRamHashTable)) {
omrthread_rwmutex_enter_write(vm->sharedClassConfig->romToRamHashTableMutex);
RomToRamEntry entry;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't strictly legal C code: declarations must be at the beginning of a block.

Moving the initialization of entry before acquiring the mutex also slightly reduces the size of that critical section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Keith, I will open a pull request to update this.

@theresa-m theresa-m mentioned this pull request Sep 19, 2024
Merged
hangshao0
hangshao0 reviewed Sep 19, 2024
View reviewed changes
runtime/shared_common/shrinit.cpp
@@ -4251,6 +4329,16 @@ j9shr_shutdown(J9JavaVM *vm)
if (utfHashTable) {
hashTableFree(utfHashTable);
}
if (NULL != romToRamHashTable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Talked to @theresa-m, it is possible that the hashtable is freed by another thread after line 4286, so this check and the following if (romToRamHashTable) { won't work as it is checking the local variable romToRamHashTable whose value might become stale already.

@hangshao0 hangshao0 mentioned this pull request Oct 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThanHenderson ThanHenderson ThanHenderson left review comments

@hangshao0 hangshao0 hangshao0 approved these changes

@keithc-ca keithc-ca keithc-ca left review comments

Assignees
No one assigned
Labels
comp:vm
Projects
None yet
Milestone
Release 0.48 (Java 8, 11, 17, 21, 23)...
Development

Successfully merging this pull request may close these issues.
5 participants
@theresa-m @hangshao0 @keithc-ca @ThanHenderson @pshipton