8329750: Change Universe functions to return more specific Klass* types #18652

stefank · 2024-04-05T11:56:11Z

We have various functions in Universe that returns Klass* where they could be returning TypeArrayKlass* and ObjArrayKlass* instead. If we change these functions we could get rid of some casts in the code. Does this seem like a reasonable change?

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-8329750: Change Universe functions to return more specific Klass* types (Enhancement - P4)

Reviewers

Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18652

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-05T11:57:07Z

👋 Welcome back stefank! 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 · 2024-04-05T11:57:37Z

@stefank 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:

8329750: Change Universe functions to return more specific Klass* types

Reviewed-by: coleenp

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 22 new commits pushed to the master branch:

3ebf8c9: 8329663: hs_err file event log entry for thread adding/removing should print current thread
be45de1: 8328627: JShell documentation should be clearer about "remote runtime system"
8648890: 8329749: Obsolete the unused UseNeon flag
fc18201: 8327111: Replace remaining usage of create_bool_from_template_assertion_predicate() which requires additional OpaqueLoop*Nodes transformation strategies
7c66465: 8325088: Overloads that differ in type parameters may be lost
6f087cb: 8328698: oopDesc::klass_raw() decodes without a null check
d1aad71: 8321204: C2: assert(false) failed: node should be in igvn hash table
51b0abc: 8329340: Remove unused libawt code
3a3b77d: 8329641: RISC-V: Enable some tests related to SHA-2 instrinsic
d771ec6: 8329733: Update the documentation in java.net.SocketOptions to direct to java.net.StandardSocketOptions
... and 12 more: https://git.openjdk.org/jdk/compare/71d48bcc3d6313ab4bd031b5e50ae3a16338abc8...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 · 2024-04-05T11:57:56Z

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

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.

mlbridge · 2024-04-05T12:01:17Z

Webrevs

coleenp

Yes, this looks really good!

src/hotspot/share/classfile/systemDictionary.cpp

dean-long · 2024-04-05T22:10:46Z

src/hotspot/share/classfile/systemDictionary.cpp

    k = Universe::typeArrayKlass(t);
-    k = TypeArrayKlass::cast(k)->array_klass(ndims, CHECK_NULL);
+    k = k->array_klass(ndims, CHECK_NULL);


I assume the cast was an attempt to de-virtualize the array_klass() call, so it is better not to use Klass* here.

My experience is that these type of casts doesn't make the compiler devirtualize the calls. I tried it now and verified that both with and without the cast we still get the virtual call. You typically need to tell the compiler what function it should be using. (I played around a lot with this when writing the devirtualization layer for the oop_iterate/OopIterateClosure code.)

I tested writing the code above as TypeArrayKlass::cast(k)->TypeArrayKlass::array_klass(ndims, CHECK_NULL) and that gets rid of the virtual call. However, the compiler still can't inline the code ArrayKlass::array_klass code because it is inside a .cpp file and not an .inline.hpp, so this results in a direct call instead of inlined code.

OK, I guess the compiler needs to be conservative in case TypeArrayKlass has a subclass.

Dean's suggestion Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>

coleenp

This change just makes it confusing and doesn't add anything. If array_klass changed more, then not sure what to do with these lines.

coleenp · 2024-04-08T13:23:29Z

src/hotspot/share/classfile/systemDictionary.cpp

-    k = TypeArrayKlass::cast(k)->array_klass(ndims, CHECK_NULL);
+    TypeArrayKlass* tak = Universe::typeArrayKlass(t);
+    k = tak->array_klass(ndims, CHECK_NULL);
+    k = k->array_klass(ndims, CHECK_NULL);


this looks puzzling. Why are there two array_klass calls now? Add short comments to explain.

I sort of see now but am getting squint lines. It's not important for performance to eliminate a virtual call here.

Hmm. Having two array_klass calls were not intentional. I accepted Dean's suggestion in the GitHub UI, but that didn't remove the old array_klass. I think I'll revert that change given that it is not important to devirtualize this.

Oh good because I was going to need a lot more coffee to understand why there was a second call. Thanks.

Why devirtualize elsehwere but not here? Maybe it's not a big deal.

Or to put it another way, what's the advantage of using TypeArrayKlass* tak in similar situations below?

This reverts commit d36f650.

coleenp · 2024-04-08T14:02:42Z

src/hotspot/share/classfile/systemDictionary.cpp

-    k = TypeArrayKlass::cast(k)->array_klass(ndims, CHECK_NULL);
+    TypeArrayKlass* tak = Universe::typeArrayKlass(t);
+    k = tak->array_klass(ndims, CHECK_NULL);
+    k = k->array_klass(ndims, CHECK_NULL);


Oh good because I was going to need a lot more coffee to understand why there was a second call. Thanks.

stefank · 2024-04-09T12:30:26Z

Thanks for the reviews! Dean, I reverted the suggestion to go with the typed TypeArrayKlass given that it had no visible effects on inlining. If you still want it I can fix it in separate commit.
/integrate

openjdk · 2024-04-09T12:31:17Z

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

87131fb: 8329629: GC interfaces should work directly against nmethod instead of CodeBlob
5ea21c3: 8329878: Reduce public interface of CardTableBarrierSet
a48289a: 8329761: Remove unused KeyBuilder and unusedSets from StyleContext
8907eda: 8325485: IncrementInstructions.of(int, int) is not storing the args
b9331cd: 8329823: RISC-V: Need to sync CPU features with related JVM flags
71c5bbc: 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType
58e39c1: 8329884: Serial: Fix build failure due to ‘Copy’ has not been declared
19a99d0: 8326744: Class-File API transition to Second Preview
b4dddde: 8329840: Fix ZPhysicalMemorySegment::_end type
dfaf11a: 8329898: Revert one use of markWord.is_unlocked back to is_neutral
... and 34 more: https://git.openjdk.org/jdk/compare/71d48bcc3d6313ab4bd031b5e50ae3a16338abc8...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-09T12:31:23Z

@stefank Pushed as commit 492b954.

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

dean-long · 2024-04-10T01:56:41Z

Thanks for the reviews! Dean, I reverted the suggestion to go with the typed TypeArrayKlass given that it had no visible effects on inlining. If you still want it I can fix it in separate commit. /integrate

If there's no benefit, then it would just be for consistency with the other changes. It's not a big deal though.

8329750: Change Universe functions to return more specific Klass* types

fc2a4a9

openjdk bot added the rfr Pull request is ready for review label Apr 5, 2024

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Apr 5, 2024

coleenp approved these changes Apr 5, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 5, 2024

dean-long reviewed Apr 5, 2024

View reviewed changes

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

dean-long reviewed Apr 5, 2024

View reviewed changes

Update src/hotspot/share/classfile/systemDictionary.cpp

d36f650

Dean's suggestion Co-authored-by: Dean Long <17332032+dean-long@users.noreply.github.com>

coleenp suggested changes Apr 8, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Apr 8, 2024

Revert "Update src/hotspot/share/classfile/systemDictionary.cpp "

36bef54

This reverts commit d36f650.

coleenp approved these changes Apr 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 8, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 9, 2024

openjdk bot closed this Apr 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329750: Change Universe functions to return more specific Klass* types #18652

8329750: Change Universe functions to return more specific Klass* types #18652

stefank commented Apr 5, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 5, 2024

openjdk bot commented Apr 5, 2024 •

edited

Loading

openjdk bot commented Apr 5, 2024

mlbridge bot commented Apr 5, 2024 •

edited

Loading

coleenp left a comment

dean-long Apr 5, 2024

stefank Apr 8, 2024

dean-long Apr 10, 2024

coleenp left a comment

coleenp Apr 8, 2024

coleenp Apr 8, 2024

stefank Apr 8, 2024

coleenp Apr 8, 2024

dean-long Apr 10, 2024

dean-long Apr 10, 2024

coleenp Apr 8, 2024

stefank commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

dean-long commented Apr 10, 2024

8329750: Change Universe functions to return more specific Klass* types #18652

8329750: Change Universe functions to return more specific Klass* types #18652

Conversation

stefank commented Apr 5, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 5, 2024

openjdk bot commented Apr 5, 2024 • edited Loading

openjdk bot commented Apr 5, 2024

mlbridge bot commented Apr 5, 2024 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefank commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

dean-long commented Apr 10, 2024

stefank commented Apr 5, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 5, 2024 •

edited

Loading

mlbridge bot commented Apr 5, 2024 •

edited

Loading