8349860: Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native #23572

coleenp · 2025-02-11T20:56:39Z

Class.isInterface() can check modifier flags, Class.isArray() can check whether component mirror is non-null and Class.isPrimitive() needs a new final transient boolean in java.lang.Class that the JVM code initializes.
Tested with tier1-4 and performance tests.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8349860: Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native (Enhancement - P4)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer) Review applies to db7c9782
Vladimir Ivanov (@iwanowww - Reviewer) Review applies to 02347433
Yudi Zheng (@mur47x111 - Committer) Review applies to 02347433
Chen Liang (@liach - Reviewer) Review applies to db7c9782
Dean Long (@dean-long - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23572/head:pull/23572
$ git checkout pull/23572

Update a local copy of the PR:
$ git checkout pull/23572
$ git pull https://git.openjdk.org/jdk.git pull/23572/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23572

View PR using the GUI difftool:
$ git pr show -t 23572

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23572.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-11T20:57:13Z

👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-02-11T20:58:08Z

@coleenp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8349860: Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native

Reviewed-by: dlong, rriggs, vlivanov, yzheng, liach

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 124 new commits pushed to the master branch:

39cb493: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
ddb2569: 8280682: Refactor AOT code source validation checks
65f79c1: 8347335: ZGC: Use limitless mark stack memory
e410af0: 8342393: Promote commutative vector IR node sharing
f755fad: 8349653: Clarify the docs for MemorySegment::reinterpret
a5c9a4d: 8349032: C2: Parse Predicate refactoring in Loop Unswitching broke fix for JDK-8290850
302bed0: 8350499: Minimal build fails with slowdebug builds
0795d11: 8350464: The flags to set the native priority for the VMThread and Java threads need a broader range
05b4812: 8350041: Skip test/jdk/java/lang/String/nativeEncoding/StringPlatformChars.java on static JDK
a891630: 8350480: RISC-V: Relax assertion about registers in C2_MacroAssembler::minmax_fp
... and 114 more: https://git.openjdk.org/jdk/compare/ed17c55ea34b3b6009dab11d64f21e0b7af3d701...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-02-11T20:58:39Z

@coleenp The following labels will be automatically applied to this pull request:

core-libs
graal
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

src/java.base/share/classes/jdk/internal/reflect/Reflection.java

...g/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java

test/jdk/jdk/internal/reflect/Reflection/Filtering.java

liach · 2025-02-12T15:49:22Z

We often need to determine what primitive type a class is. Currently we do it through Wrapper.forPrimitiveType. Do you see potential value in encoding the primitive status in a byte, so primitive info also knows what primitive type this class is instead of doing identity comparisons?

coleenp · 2025-02-14T13:53:43Z

I had a look at Wrapper.forPrimitiveType() and it's not an intrinsic so I don't really know how hot it is. It's a comparison, vs getting a field out of Class<?>. Not sure how to measure it. So I can't address it in this change.

liach · 2025-02-14T15:20:00Z

@cl4es Can you offer some insight here?

cl4es · 2025-02-17T04:32:48Z

Touching Wrapper seems out of scope for this PR, but if Class.isPrimitive gets cheaper from this then Wrapper.forPrimitiveType should definitely be examined in a follow-up.

mlbridge · 2025-02-18T23:49:52Z

Webrevs

src/hotspot/share/classfile/javaClasses.inline.hpp

src/java.base/share/classes/java/lang/Class.java

dean-long · 2025-02-19T03:35:02Z

src/hotspot/share/prims/jvm.cpp

 // Otherwise it returns its argument value which is the _the_class Klass*.
-// Please, refer to the description in the jvmtiThreadSate.hpp.
+// Please, refer to the description in the jvmtiThreadState.hpp.
+


Does this "RedefineClasses support" comment still belong here?

I think so. The comment in jvmtiThreadState.hpp has details why this is. We do a mirror switch before verification apparently because of bug 6214132 it says.

dholmes-ora

Just a few passing comments as this is mainly compiler stuff.

Does the SA not need any updates in relation to this?

src/hotspot/share/classfile/javaClasses.cpp

src/hotspot/share/prims/jvm.cpp

src/java.base/share/classes/java/lang/Class.java

RogerRiggs

Is the change to isInterface and isPrimitive performance neutral?
As @IntrinsicCandidates, there would be some performance gain.

RogerRiggs · 2025-02-19T15:07:57Z

src/hotspot/share/prims/jvm.cpp

-// Please, refer to the description in the jvmtiThreadSate.hpp.
+// Please, refer to the description in the jvmtiThreadState.hpp.
+
+JVM_ENTRY(jboolean, JVM_IsInterface(JNIEnv *env, jclass cls))


JVM_IsInteface is deleted in Class.c, what purpose is this?

The old classfile verifier uses JVM_IsInterface.

src/java.base/share/classes/java/lang/Class.java

liach · 2025-02-19T16:22:48Z

Re roger's IntrinsicCandidate remark: One behavior that might be affected would be C2's inlining preferences. Some inline-sensitive workloads like FFM API might be affected if some Class attribute access cannot be inlined because the incoming Class object is not constant. See #23460 and #23628.

coleenp

Thanks for looking at this change.

src/hotspot/share/classfile/javaClasses.cpp

src/hotspot/share/classfile/javaClasses.inline.hpp

src/hotspot/share/prims/jvm.cpp

coleenp · 2025-02-19T17:03:25Z

src/hotspot/share/prims/jvm.cpp

 // Otherwise it returns its argument value which is the _the_class Klass*.
-// Please, refer to the description in the jvmtiThreadSate.hpp.
+// Please, refer to the description in the jvmtiThreadState.hpp.
+


I think so. The comment in jvmtiThreadState.hpp has details why this is. We do a mirror switch before verification apparently because of bug 6214132 it says.

src/java.base/share/classes/java/lang/Class.java

Co-authored-by: David Holmes <62092539+dholmes-ora@users.noreply.github.com>

iwanowww

Looks good!

Regarding @IntrinsicCandidate and its effects on JIT-compiler inlining decisions, @ForceInline could be added, but IMO it's not necessary since new implementations are small.

src/java.base/share/classes/java/lang/Class.java

coleenp · 2025-02-20T23:29:29Z

Thanks Vladimir for review and for answering my earlier questions on this change.

mur47x111

LGTM! As @iwanowww said, not inlining such trivial methods seems more like an inliner bug/enhancement opportunity.

coleenp · 2025-02-24T15:56:48Z

Does the SA not need any updates in relation to this?

No, the SA doesn't know about these compiler intrinsics.

liach

The limited changes to the Java codebase looks reasonable. We should probably get a double check from Alan or some other architect.

RogerRiggs

A nice simplification.

src/java.base/share/classes/java/lang/Class.java

coleenp · 2025-02-24T19:27:38Z

Thanks for reviewing Roger.

coleenp · 2025-02-25T12:34:40Z

Thanks for reviewing Dean, Roger, Vladimir, Yudi and Chen, and comments David.
/integrate

openjdk · 2025-02-25T12:35:54Z

Going to push as commit c413549.
Since your change was applied there have been 131 commits pushed to the master branch:

a9c9f7f: 8192647: GClocker induced GCs can starve threads requiring memory leading to OOME
aa70f0a: 8347348: Clarify that the HTTP server in jdk.httpserver module is not a full featured server
d551dac: 8350103: Test containers/systemd/SystemdMemoryAwarenessTest.java fails on Linux ppc64le SLES15 SP6
ab86a13: 8350548: java.lang.classfile package javadoc has errors
e1081cf: 8348536: Remove remain SIZE_FORMAT usage after JDK-8347990
a6cc37f: 8349888: AOTMode=create crashes with EpsilonGC
990d40e: 8350476: Fix typo introduced in JDK-8350147
39cb493: 8348106: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon
ddb2569: 8280682: Refactor AOT code source validation checks
65f79c1: 8347335: ZGC: Use limitless mark stack memory
... and 121 more: https://git.openjdk.org/jdk/compare/ed17c55ea34b3b6009dab11d64f21e0b7af3d701...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-25T12:36:01Z

@coleenp Pushed as commit c413549.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot changed the title ~~8349860 Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native~~ 8349860: Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native Feb 11, 2025

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Feb 11, 2025

liach reviewed Feb 12, 2025

View reviewed changes

coleenp added 4 commits February 12, 2025 12:10

Make isArray non-native

f0133fc

Make isInterface non-native.

072f332

Remove isPrimitive intrinsic in favor of a boolean.

c0e9919

Shrink modifiers flag so isPrimitive can share word.

e8119a2

coleenp force-pushed the isArray branch from b17e91a to e8119a2 Compare February 12, 2025 12:22

graalvmbot mentioned this pull request Feb 17, 2025

[JDK-8350205] Adapt JDK-8349860: Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native oracle/graal#10704

Closed

Add ')' removed from jvmci test.

2d9b9ff

coleenp marked this pull request as ready for review February 18, 2025 23:43

openjdk bot added the rfr Pull request is ready for review label Feb 18, 2025

dean-long reviewed Feb 19, 2025

View reviewed changes

src/hotspot/share/classfile/javaClasses.inline.hpp Show resolved Hide resolved

dean-long reviewed Feb 19, 2025

View reviewed changes

src/java.base/share/classes/java/lang/Class.java Show resolved Hide resolved

dean-long reviewed Feb 19, 2025

View reviewed changes

dholmes-ora reviewed Feb 19, 2025

View reviewed changes

src/hotspot/share/classfile/javaClasses.cpp Show resolved Hide resolved

src/hotspot/share/prims/jvm.cpp Show resolved Hide resolved

src/java.base/share/classes/java/lang/Class.java Show resolved Hide resolved

RogerRiggs reviewed Feb 19, 2025

View reviewed changes

src/java.base/share/classes/java/lang/Class.java Show resolved Hide resolved

coleenp commented Feb 19, 2025

View reviewed changes

Update src/java.base/share/classes/java/lang/Class.java

7a4c595

Co-authored-by: David Holmes <62092539+dholmes-ora@users.noreply.github.com>

openjdk bot removed the rfr Pull request is ready for review label Feb 20, 2025

Fix whitespace

0234743

openjdk bot added the rfr Pull request is ready for review label Feb 20, 2025

iwanowww approved these changes Feb 20, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 20, 2025

JnRouvignac reviewed Feb 20, 2025

View reviewed changes

src/java.base/share/classes/java/lang/Class.java Outdated Show resolved Hide resolved

mur47x111 approved these changes Feb 21, 2025

View reviewed changes

Remove JVM_GetClassModifiers from jvm.h too.

c23718b

openjdk bot removed the ready Pull request is ready to be integrated label Feb 21, 2025

Use modifiers field directly in isInterface.

db7c978

liach approved these changes Feb 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 24, 2025

RogerRiggs approved these changes Feb 24, 2025

View reviewed changes

src/java.base/share/classes/java/lang/Class.java Show resolved Hide resolved

Add a comment about Class constructor.

591abdd

openjdk bot removed the ready Pull request is ready to be integrated label Feb 24, 2025

dean-long approved these changes Feb 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 24, 2025

openjdk bot added the integrated Pull request has been integrated label Feb 25, 2025

openjdk bot closed this Feb 25, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 25, 2025

coleenp deleted the isArray branch February 25, 2025 12:37

graalvmbot mentioned this pull request Mar 3, 2025

[GR-62646] Update labsjdk to 25+12-jvmci-b01 oracle/graal#10803

Merged

8349860: Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native #23572

8349860: Make Class.isArray(), Class.isInterface() and Class.isPrimitive() non-native #23572

Uh oh!

Conversation

coleenp commented Feb 11, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 11, 2025

Uh oh!

openjdk bot commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liach commented Feb 12, 2025

Uh oh!

coleenp commented Feb 14, 2025

Uh oh!

liach commented Feb 14, 2025

Uh oh!

cl4es commented Feb 17, 2025

Uh oh!

mlbridge bot commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

dean-long Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

RogerRiggs Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liach commented Feb 19, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coleenp Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coleenp commented Feb 20, 2025

Uh oh!

mur47x111 left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Feb 24, 2025

coleenp commented Feb 11, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 11, 2025 •

edited

Loading

mlbridge bot commented Feb 18, 2025 •

edited

Loading