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

Refactor J9JVMTI_HEAP_EVENT_STACK data parsing #18864

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

fengxue-IS
Copy link
Contributor

Added new structure to hold JNI and Stack local referrer data. Update JNI locals found via jniLocalReferences table to be marked as Stack root.

Closes: #18416

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Sep 3, 2024

#19486 reverted some of the changes in #18180 which properly handle reference chain walking on virtual thread stack.
This change corrects the behaviour by caching the virtual thread object in ReferenceChainWalker and fill the value into J9MM_StackSlotDescriptor->vmThread before executing the callback function.

@fengxue-IS fengxue-IS marked this pull request as ready for review September 3, 2024 21:42
@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Sep 3, 2024

@babsingh @LinHu2016 can you please take a look at this
local testing passed, personal build passed
Note VThreadStackRefTest.java is still excluded atm

runtime/gc_base/ReferenceChainWalker.cpp Outdated Show resolved Hide resolved
runtime/gc_base/ReferenceChainWalker.cpp Outdated Show resolved Hide resolved
runtime/gc_base/ReferenceChainWalker.cpp Outdated Show resolved Hide resolved
runtime/gc_base/ReferenceChainWalker.cpp Outdated Show resolved Hide resolved
runtime/gc_base/ReferenceChainWalker.hpp Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHeap10.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHeap10.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHeap10.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHeap10.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHeap10.c Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

babsingh commented Sep 4, 2024

Consistent formatting

There are places where there is space before and after *. Others only have space before *; there is no space after *. The latter is the preferred approach. If we're making small changes, then we should follow the existing formatting used in the file. If we're changing majority of the function, then should update the function to follow the preferred approach.

processStackRoot(J9JVMTIObjectIteratorData * data, J9JVMTIObjectTag * entry, jlong size, jlong classTag, jvmtiHeapRootKind kind, J9MM_StackSlotDescriptor *stackSlotDescriptor)

Similarly, if we're adding a cast, then the preferred approach is to not have space after the closing bracket ). Use the above logic to either employ existing formatting or transition to the preferred approach.

slot = (jint) walkState->slotIndex;

@babsingh
Copy link
Contributor

babsingh commented Sep 4, 2024

How have these changes been tested? Were any personal builds run? If so, which test suites were selected?

@fengxue-IS
Copy link
Contributor Author

How have these changes been tested? Were any personal builds run? If so, which test suites were selected?

I've include link to personal build, see above #18864 (comment)

@babsingh
Copy link
Contributor

babsingh commented Sep 4, 2024

local testing passed, personal build passed

Are any of the failures and unstable builds related to this PR?

Note VThreadStackRefTest.java is still excluded atm

Does VThreadStackRefTest.java pass on all platforms?

fengxue-IS added a commit to fengxue-IS/aqa-tests that referenced this pull request Sep 6, 2024
Test failure have been resolved by eclipse-openj9/openj9#18864

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Sep 6, 2024

local testing passed, personal build passed

Are any of the failures and unstable builds related to this PR?

No

Note VThreadStackRefTest.java is still excluded atm

Does VThreadStackRefTest.java pass on all platforms?

I've only tested on xlinux and amac for JDK 21, but since the test are all testing JVM internals, there shouldn't be any platform related differences.
I've opened adoptium/aqa-tests#5567 to re-enable the test

@LinHu2016
Copy link
Contributor

LGTM, need to confirm ReferenceChainWalker is not caller during concurrent scavenger(currently it is only used during STW GC, read threadObject via Java API during concurrent scavenger might cause an infinity loop).

Added new structure to hold JNI and Stack local referrer data.
Update JNI locals found via jniLocalReferences table to be marked as Stack root.

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Copy link
Contributor Author

@LinHu2016 added assertions to ensure API only used under exclusive access

@fengxue-IS fengxue-IS force-pushed the 18416-jni-local branch 2 times, most recently from 3d88a08 to 711194c Compare October 15, 2024 22:33
@babsingh
Copy link
Contributor

jenkins test sanity,extended.openjdk zlinux jdk21

@babsingh
Copy link
Contributor

jenkins test sanity alinux jdk11

@babsingh
Copy link
Contributor

There are compilation errors on JDK11. vthread code is missing ifdefs.

11:38:59  /home/jenkins/workspace/Build_JDK11_aarch64_linux_Personal/openj9/runtime/gc_base/ReferenceChainWalker.cpp: In function 'void j9gc_ext_reachable_objects_do(J9VMThread*, jvmtiIterationControl (*)(J9Object**, J9Object*, void*, IDATA, IDATA, IDATA), void*, uintptr_t)':
11:38:59  /home/jenkins/workspace/Build_JDK11_aarch64_linux_Personal/openj9/runtime/gc_base/ReferenceChainWalker.cpp:95:24: error: 'class MM_ReferenceChainWalker' has no member named 'includeVThreadObject'
11:38:59     95 |   referenceChainWalker.includeVThreadObject();
11:38:59        |                        ^~~~~~~~~~~~~~~~~~~~
11:38:59  /home/jenkins/workspace/Build_JDK11_aarch64_linux_Personal/openj9/runtime/gc_base/ReferenceChainWalker.cpp: In function 'void j9gc_ext_reachable_from_object_do(J9VMThread*, J9Object*, jvmtiIterationControl (*)(J9Object**, J9Object*, void*, IDATA, IDATA, IDATA), void*, uintptr_t)':
11:38:59  /home/jenkins/workspace/Build_JDK11_aarch64_linux_Personal/openj9/runtime/gc_base/ReferenceChainWalker.cpp:129:24: error: 'class MM_ReferenceChainWalker' has no member named 'includeVThreadObject'
11:38:59    129 |   referenceChainWalker.includeVThreadObject();
11:38:59        |                        ^~~~~~~~~~~~~~~~~~~~
11:38:59  runtime/gc_base/CMakeFiles/j9gcbase.dir/build.make:335: recipe for target 'runtime/gc_base/CMakeFiles/j9gcbase.dir/ReferenceChainWalker.cpp.o' 

- Cache and fill in virtual thread object for jvmtiFollowReferences calls
- Add exclusive VM access assertion check to j9gc_ext calls

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Copy link
Contributor Author

Added ifdef to fix JDK11 compile error, the JDK 21 PR build also failed due to jenkins flow error, @babsingh can you restart the builds please

@babsingh
Copy link
Contributor

jenkins test sanity,extended.openjdk zlinux jdk21

@babsingh
Copy link
Contributor

jenkins test sanity win,alinux jdk11

@babsingh
Copy link
Contributor

jenkins test sanity win jdk21

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.

LGTM. Test_openjdk21_j9_extended.openjdk PR build has unrelated security/SSL failures.

@babsingh babsingh merged commit 64cb0db into eclipse-openj9:master Oct 17, 2024
15 of 16 checks passed
fengxue-IS added a commit to fengxue-IS/aqa-tests that referenced this pull request Nov 14, 2024
Test failure have been resolved by eclipse-openj9/openj9#18864

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix JVMTI_HEAP_REFERENCE_JNI_LOCAL scan/iterate code
3 participants