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

Force inline AbstractMemorySegmentImpl.reinterpret for getCallerClass optimization #20471

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

nbhuiyan
Copy link
Member

AbstractMemorySegmentImpl.reinterpret methods contain calls to
Reflection.getCallerClass. Optimization of getCallerClass requires
the caller to be inlined. Otherwise, we would end up using the
JNI helper which performs a very expensive stackwalk to obtain
the caller class.

Issue: #20270

@nbhuiyan
Copy link
Member Author

@hzongaro / @0xdaryl, would either one of you be able to review this please?

@@ -366,6 +366,9 @@ TR_J9InlinerPolicy::alwaysWorthInlining(TR_ResolvedMethod * calleeMethod, TR::No
case TR::java_lang_Class_newInstance:
case TR::jdk_internal_util_Preconditions_checkIndex:

// AbstractMemorySegmentImpl.reinterpret methods call Reflection.getCallerClass
case TR::jdk_internal_foreign_AbstractMemorySegmentImpl_reinterpret:
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this only need to be done for JDK22+? If so, this case should be guarded with a JDK level check.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I don't think we usually add conditional compilation for methods that exist in some versions of the Java Class Libraries but not others. Unless there's really a difference in the behaviour for different versions — as opposed to a difference in whether it exists — I don't think this needs to be conditionally compiled in or out.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was the reasoning behind my question: whether there was a difference in behaviour between 21 and 22+ and if you only need to affect behaviour this way for JDK22+. If the answer is no (and I just now compared the code myself and see that they are largely similar) then guarding it for a JDK level is not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

@0xdaryl I misunderstood your question and added the guard. I have now removed that change.

AbstractMemorySegmentImpl.reinterpret methods contain calls to
Reflection.getCallerClass. Optimization of getCallerClass requires
the caller to be inlined. Otherwise, we would end up using the
JNI helper which performs a very expensive stackwalk to obtain
the caller class.

Signed-off-by: Nazim Bhuiyan <[email protected]>
@0xdaryl
Copy link
Contributor

0xdaryl commented Nov 5, 2024

Jenkins test sanity all jdk21

@0xdaryl 0xdaryl self-assigned this Nov 5, 2024
@0xdaryl 0xdaryl merged commit 33c0833 into eclipse-openj9:master Nov 5, 2024
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants