openj9
openj9 copied to clipboard
eclipse-openj9
Add thread local storage to java thread objects
- Add an array hidden field to java/lang/Thread
- Add and implement jvmti TLS functions to store env-thread-pair TLS data
- Add tls pool to vm to allocate threads' tls arrays from
- Add and implement hook events for virtual thread create/destroy. They are hooked for each jvmti env created and allocate/destroy the thread data block for the hooked jvmti env when a virtual thread starts/terminates.
Issue: #15183 Signed-off-by: Eric Yang [email protected]
Test: https://github.com/ibmruntimes/openj9-openjdk-jdk19/tree/openj9/test/hotspot/jtreg/serviceability/jvmti/stress/ThreadLocalStorage/SetGetThreadLocalStorageStressTest
The above test covers virtual threads, platform threads, event callbacks, event notification modes and thread local storage. It should succeed with this PR.
I was planning to leave the usage of the tls functions and hooks/allocations to a third pr once the current two have been merged since this pr requires the hidden field, native methods, and the virtual thread linked list.
Also I thought we were going to use the new TLS for all java versions. I am adding new changes such as THREAD_DATA_FOR_THREAD_OBJECT and createTLSData just as a placeholder for now, in the third pr I will get replace the old code. This is so I don't have to implement everything in this pr (e.g. updating createThreadData will force me to add changes to allocateEnvironment which I don't have the virtual thread list to do.
This PR should fix the following JVMTI functions in JDK19: SetThreadLocalStorage, GetThreadLocalStorage, SetEventCallbacks, SetEventNotificationMode and JVMTI event callbacks (J9JVMTIThreadData->threadEventEnable). My preference will be to have these functions fully working in this PR.
allocateEnvironment which I don't have the virtual thread list to do
The TLS will only be used in the above listed methods. We don't need to allocate J9JVMTIThreadData for all existing threads in allocateEnvironment. We can just allocate it in the Set* methods. The Get* methods will return the default value if it has not been allocated. This will improve the perf for allocateEnvironment and give us better startup perf.
Then, the virtual thread list will only be needed by SuspendAllVirtualThreads and ResumeAllVirtualThreads. The two PRs will be completely independent.
See https://github.com/eclipse-openj9/openj9/issues/15183#issuecomment-1183345480 and surrounding comments for more details. This approach was preferred by @gacholio.
Also I thought we were going to use the new TLS for all java versions.
Memory footprint will increase. Perf team will notice it and tag it as a regression for JDK 8/11/17. This suggestion would work if we completely disable the existing TLS for J9Thread (OS thread). But there will be other issues, which will need to be addressed, for disabling the TLS for J9Thread (OS thread). We can discuss about them later; not a priority since our current focus is JVMTI functionality. To avoid a memory footprint regression, we should only enable the new TLS for JDK19+.
the virtual thread list will only be needed by SuspendAllVirtualThreads and ResumeAllVirtualThreads
I am not sure about this, two more recent comments mention allocate/freeing in the virtual thread natives. https://github.com/eclipse-openj9/openj9/issues/15183#issuecomment-1189079601 https://github.com/eclipse-openj9/openj9/issues/15183#issuecomment-1189406766
@gacholio @tajila
For virtual threads we are allocating/deallocating the tls array at notifyJvmtiMountBegin and notifyJvmtiUnmountEnd. Where do we do this for platform threads? allocateVMThread/deallocateVMThread?
Java 19 sanity.functional passed: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/13890/
I also tested locally with a GetThreadLocalStorage -> SetThreadLocalStorage -> GetThreadLocalStorage for platform and virtual threads and they return the correct values.
This PR is ready for review
Does the following test pass: https://github.com/ibmruntimes/openj9-openjdk-jdk19/tree/openj9/test/hotspot/jtreg/serviceability/jvmti/stress/ThreadLocalStorage/SetGetThreadLocalStorageStressTest?
Also, can you point me to the code that sets targetThread = currentThread for a yielded virtual thread?
Also, can you point me to the code that sets targetThread = currentThread for a yielded virtual thread?
I will do that after your changes get merged; it will be done after getVMThread. For jdk19+, targetThread is only used to get the VM so it just needs to be non null. Currently if getVMThread succeeds targetThread will always be non null.
On leave till 31 Aug 22; won't be able to complete the review. Delegating the review to either @tajila or @gacholio or @hangshao0.
Fixed merge conflicts, ran some tests locally and passed. PR is ready for final review @gacholio @tajila
Looks good - almost all my comments are just doc-related. Documentation for non-static functions should go only in the header file that prototypes them (to avoid multiple copies of the doc diverging over time).
https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/13988/ https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/13989/ xlinux 19 and 17 tests
PR tests: https://openj9-jenkins.osuosl.org/job/Pipeline_Build_Test_JDK19_x86-64_windows/103/ https://openj9-jenkins.osuosl.org/job/Pipeline_Build_Test_JDK8_x86-64_windows/1212/
Fixing merge conflict from getThreadInfo PR (j9nonbuilder.h)
JDK8 passed JDK19 passed