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

Recognize JNI local refs in JNINativeMethodFrames #18378

Merged
merged 1 commit into from Oct 31, 2023
Merged

Recognize JNI local refs in JNINativeMethodFrames #18378

merged 1 commit into from Oct 31, 2023

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented Oct 30, 2023 •
edited

First 8 JNI local refs are directly stored inside the J9SFJNINativeMethodFrame before allocating J9JNIReferenceFrame to store additional references. StackWalker should check if the object pushes from method frame is JNI local when walking O-Slots.

Related: #17712

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

@fengxue-IS
Copy link
Contributor Author

There is a dependent change from GC side from @LinHu2016 to correctly mark slots returned based on vm supplied walkState->slotType in ReferenceChainWalker.

@babsingh
Copy link
Contributor

Changes LGTM. @fengxue-IS Can you include the below justification in the commit message?

First 8 JNI local refs are directly stored inside the
J9SFJNINativeMethodFrame before allocating J9JNIReferenceFrame to
store additional references. StackWalker should check if the object
pushes from method frame is JNI local when walking O-Slots.

Related: #17712

@babsingh
Copy link
Contributor

There is a dependent change from GC side from @LinHu2016 to correctly mark slots returned based on vm supplied walkState->slotType in ReferenceChainWalker.

Will we need to wait for the GC change before merging this PR?

@LinHu2016 LinHu2016 mentioned this pull request Oct 30, 2023
Merged
@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Oct 30, 2023 •
edited

There is a dependent change from GC side from @LinHu2016 to correctly mark slots returned based on vm supplied walkState->slotType in ReferenceChainWalker.

Will we need to wait for the GC change before merging this PR?

no, just need the GC changes to pass the test.

@fengxue-IS
Recognize JNI local refs in JNINativeMethodFrames
cec8a15 
First 8 JNI local refs are directly stored inside the
J9SFJNINativeMethodFrame before allocating J9JNIReferenceFrame to
store additional references. StackWalker should check if the object
pushes from method frame is JNI local when walking O-Slots.

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

jenkins test sanity amac jdk21

@babsingh
Copy link
Contributor

jenkins compile win jdk21

@babsingh
Copy link
Contributor

jenkins test sanity.openjdk,extended.openjdk amac jdk21

1 similar comment
@babsingh
Copy link
Contributor

jenkins test sanity.openjdk,extended.openjdk amac jdk21

@babsingh
Copy link
Contributor

babsingh commented Oct 31, 2023 •
edited

Launching new PR builds on a different platform due to infra issues:

jenkins test sanity,sanity.openjdk,extended.openjdk alinux jdk21

@babsingh
Copy link
Contributor

@babsingh babsingh merged commit 3a5a8e6 into eclipse-openj9:master Oct 31, 2023
6 of 12 checks passed
@fengxue-IS fengxue-IS mentioned this pull request Nov 1, 2023
Merged
@dmitripivkine dmitripivkine mentioned this pull request Nov 2, 2023
Closed
@fengxue-IS fengxue-IS mentioned this pull request Nov 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@babsingh babsingh babsingh 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
2 participants
@fengxue-IS @babsingh