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

Add thread local storage to java thread objects #15541

Merged
merged 1 commit into from Aug 27, 2022
Merged

Add thread local storage to java thread objects #15541

merged 1 commit into from Aug 27, 2022

Conversation

EricYangIBM
Copy link
Contributor

@EricYangIBM EricYangIBM commented Jul 13, 2022 •
edited

  • Add an array hidden field to java/lang/Thread as the TLS for a given
    thread object (JDK19+). JDK18 and below will continue to use the OMR TLS
  • Implement JVMTI TLS functions to store env-thread-pair TLS data
  • Add tls pool to vm to allocate threads' tls arrays from
  • (JDK19+) Lazy allocate a thread's TLS array at first use
    (SetThreadLocalStorage and SetEventNotificationMode)
  • (JDK19+) Free the TLS array at VMThread destroy and virtual thread end
  • Lazy allocate TLS thread data blocks at first use
    (SetThreadLocalStorage and SetEventNotificationMode)
  • Implement new hook events for platform and virtual thread end to free
    the thread data block allocated by a JVMTI env

Issue: #15183
Signed-off-by: Eric Yang eric.yang@ibm.com

@EricYangIBM EricYangIBM mentioned this pull request Jul 14, 2022
Closed
@EricYangIBM EricYangIBM marked this pull request as ready for review July 14, 2022 20:16
@EricYangIBM EricYangIBM marked this pull request as draft July 25, 2022 14:16
@EricYangIBM EricYangIBM marked this pull request as ready for review July 25, 2022 19:41
babsingh
babsingh requested changes Jul 28, 2022
View reviewed changes
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved
runtime/jvmti/jvmti_internal.h Outdated Show resolved Hide resolved
runtime/oti/jvmtiInternal.h Outdated Show resolved Hide resolved
runtime/oti/jvmtiInternal.h Outdated Show resolved Hide resolved
runtime/oti/j9vm.hdf Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Show resolved Hide resolved
@babsingh
Copy link
Contributor

babsingh commented Jul 28, 2022 •
edited

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.

@EricYangIBM
Copy link
Contributor Author

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.

@babsingh
Copy link
Contributor

babsingh commented Aug 2, 2022 •
edited

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 #15183 (comment) 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+.

@EricYangIBM
Copy link
Contributor Author

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. #15183 (comment)
#15183 (comment)

@EricYangIBM
Copy link
Contributor Author

@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?

@tajila
Copy link
Contributor

tajila commented Aug 3, 2022

allocateVMThread/deallocateVMThread?

That seems like the correct spot

@babsingh babsingh mentioned this pull request Aug 4, 2022
Closed
@EricYangIBM EricYangIBM force-pushed the jvmtiTLS branch 6 times, most recently from 1e563af to 03af58f Compare August 17, 2022 17:13
@EricYangIBM
Copy link
Contributor Author

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

@babsingh
Copy link
Contributor

Does the following test pass: https://github.com/ibmruntimes/openj9-openjdk-jdk19/tree/openj9/test/hotspot/jtreg/serviceability/jvmti/stress/ThreadLocalStorage/SetGetThreadLocalStorageStressTest?

@EricYangIBM
Copy link
Contributor Author

No because it depends on GetThreadInfo

babsingh
babsingh previously requested changes Aug 19, 2022
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

High level overview:

  • Commit/PR's description should highlight the startup performance benefits on all JDK version.
  • The description should also highlight the extra memory that is needed for the TLS in JDK19+.
  • Majority of the feedback is related to avoiding redundant code.

runtime/vm/jvmfree.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHook.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmti_internal.h Outdated Show resolved Hide resolved
runtime/jvmti/jvmti_internal.h Outdated Show resolved Hide resolved
runtime/oti/jvmtiInternal.h Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

Also, can you point me to the code that sets targetThread = currentThread for a yielded virtual thread?

@EricYangIBM
Copy link
Contributor Author

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.

@babsingh
Copy link
Contributor

babsingh commented Aug 19, 2022 •
edited

On leave till 31 Aug 22; won't be able to complete the review. Delegating the review to either @tajila or @gacholio or @hangshao0.

hangshao0
hangshao0 reviewed Aug 22, 2022
View reviewed changes
runtime/jvmti/j9jvmti.tdf Show resolved Hide resolved
runtime/jvmti/j9jvmti.tdf Show resolved Hide resolved
runtime/jvmti/jvmtiEventManagement.c Show resolved Hide resolved
hangshao0
hangshao0 reviewed Aug 22, 2022
View reviewed changes
runtime/jvmti/jvmtiThread.c Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor

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).

@EricYangIBM
Copy link
Contributor Author

Updated

@gacholio
Copy link
Contributor

jenkins test sanity win jdk19,jdk8

@gacholio gacholio requested review from babsingh and removed request for babsingh August 26, 2022 18:25
@gacholio gacholio removed the request for review from babsingh August 26, 2022 18:26
@EricYangIBM
Copy link
Contributor Author

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

@EricYangIBM
Add thread local storage to java thread objects
af3a4c1 
- Add an array hidden field to java/lang/Thread as the TLS for a given
thread object (JDK19+). JDK18 and below will continue to use the OMR TLS
- Implement JVMTI TLS functions to store env-thread-pair TLS data
- Add tls pool to vm to allocate threads' tls arrays from
- (JDK19+) Lazy allocate a thread's TLS array at first use
(SetThreadLocalStorage and SetEventNotificationMode)
- (JDK19+) Free the TLS array at VMThread destroy and virtual thread end
- Lazy allocate TLS thread data blocks at first use
(SetThreadLocalStorage and SetEventNotificationMode)
- Implement new hook events for platform and virtual thread end to free
the thread data block allocated by a JVMTI env

Issue: eclipse-openj9#15183
Signed-off-by: Eric Yang <eric.yang@ibm.com>
@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Aug 26, 2022 •
edited

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

@tajila
Copy link
Contributor

tajila commented Aug 26, 2022

jenkins test sanity win jdk19

@gacholio gacholio merged commit c14fe74 into eclipse-openj9:master Aug 27, 2022
babsingh added a commit to babsingh/openj9 that referenced this pull request Sep 26, 2022
@babsingh
Fix setEventNotificationMode for unmounted virtual threads
d32a67d 
targetThread is NULL only for virtual threads, as per the assertion in
getVMThread, when mustBeAlive is TRUE. vmThreadForTLS is only used to
acquire J9JavaVM in createThreadData and jvmtiTLSGet. If targetThread
is NULL, currentThread is passed to createThreadData and jvmtiTLSGet
for retrieving J9JavaVM in JDK19+.

Related: eclipse-openj9#15541
Related: eclipse-openj9#15183

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh mentioned this pull request Sep 26, 2022
Merged
babsingh added a commit to babsingh/openj9 that referenced this pull request Sep 27, 2022
@babsingh
Fix setEventNotificationMode for unmounted virtual threads
10c34eb 
targetThread is NULL only for virtual threads, as per the assertion in
getVMThread, when mustBeAlive is TRUE. vmThreadForTLS is only used to
acquire J9JavaVM in createThreadData and jvmtiTLSGet. If targetThread
is NULL, currentThread is passed to createThreadData and jvmtiTLSGet
for retrieving J9JavaVM in JDK19+.

Related: eclipse-openj9#15541
Related: eclipse-openj9#15183

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh mentioned this pull request Mar 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@babsingh babsingh babsingh left review comments

@hangshao0 hangshao0 hangshao0 approved these changes

@gacholio gacholio gacholio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@EricYangIBM @babsingh @tajila @gacholio @hangshao0