8358326: Use oopFactory array allocation #25590

coleenp · 2025-06-02T14:07:10Z

This patch removes cases of direct calls to {type,obj}ArrayKlass->allocate() and calls oopFactory::new_*array instead. It also renames {type,obj}ArrayKlass->allocate functions to allocate_klass and allocate_instance so it's more clear which allocation it's doing and to match InstanceKlass allocate functions, and makes these functions private with friends for Deoptimization and oopFactory. For JEP 401, arrays are being extended to support new formats and attributes and this reduces the call sites.
Tested with tier1-7.

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-8358326: Use oopFactory array allocation (Enhancement - P4)

Reviewers

Stefan Karlsson (@stefank - Reviewer) Review applies to 2da2b11b
Frederic Parain (@fparain - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25590

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-02T14:08:00Z

👋 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-06-02T14:08:05Z

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

8358326: Use oopFactory array allocation

Reviewed-by: fparain, stefank

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

cae1fd3: 8357632: CDS test failures on static JDK
eb8ee8b: 8358731: Remove jdk.internal.access.JavaAWTAccess.java
2103dc1: 8358452: JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119)
... and 10 more: https://git.openjdk.org/jdk/compare/b2e7cda6a0bd21fa3c4ffe2a67da4953f1ca3f1f...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-06-02T14:08:31Z

@coleenp 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 · 2025-06-06T12:06:28Z

Webrevs

01: Full - Incremental (2ccaf422)
00: Full (2da2b11b)

stefank

This looks good to me. I a few suggestions that you could take if you want to.

stefank · 2025-06-09T07:33:32Z

src/hotspot/share/runtime/reflection.cpp

+    if (type == T_VOID) {
+      THROW_NULL(vmSymbols::java_lang_IllegalArgumentException());
+    }


I was first wondering where this came from but I now see that this was duplicated from basic_type_mirror_to_arrayklass. I wonder if this could could be deduplicated by moving this check into basic_type_mirror_to_basic_type and then removed from -basic_type_mirror_to_arrayklass:

static BasicType basic_type_mirror_to_basic_type(oop basic_type_mirror, TRAPS) { assert(java_lang_Class::is_primitive(basic_type_mirror), "just checking"); if (type == T_VOID) { THROW_NULL(vmSymbols::java_lang_IllegalArgumentException()); } return java_lang_Class::primitive_type(basic_type_mirror); } static Klass* basic_type_mirror_to_arrayklass(oop basic_type_mirror, TRAPS) { BasicType type = basic_type_mirror_to_basic_type(basic_type_mirror, CHECK_NULL); return Universe::typeArrayKlass(type); }

And then this code could be a two-liner again:

if (java_lang_Class::is_primitive(element_mirror)) { BasicType type = basic_type_mirror_to_basic_type(element_mirror, CHECK_NULL); return oopFactory::new_typeArray(type, length, CHECK_NULL); }

Unfortunately the caller to basic_type_mirror_to_basic_type() can legitimately return T_VOID for the caller in reflect_method, so that's why I had to duplicate the exception code. Maybe a future enhancement would be to move these to javaClasses.hpp in java_lang_Class, where it knows all about is_primitive types, and boxing classes, which I guess boxing T_VOID is a thing.

stefank · 2025-06-09T07:36:32Z

src/hotspot/share/oops/objArrayKlass.hpp

+  friend class oopFactory;
+  friend class Deoptimization;


If you want you could consider sorting the friend declarations (here and in the other place where you added it)

Do we sort friends? The sorting looks funny since VMStructs is usually at the beginning.

class ObjArrayKlass : public ArrayKlass { - friend class VMStructs; + friend class Deoptimization; friend class JVMCIVMStructs; friend class oopFactory; - friend class Deoptimization; + friend class VMStructs;

stefank · 2025-06-09T07:38:25Z

src/hotspot/share/oops/objArrayKlass.hpp

@@ -78,7 +82,6 @@ class ObjArrayKlass : public ArrayKlass {
  static ObjArrayKlass* allocate_objArray_klass(ClassLoaderData* loader_data,
                                                int n, Klass* element_klass, TRAPS);

-  objArrayOop allocate(int length, TRAPS);


Do you think multi_allocate will need a better name in the future?

I thought if changing multi_allocate_instance so that it's clear that it's an instance, but decided to limit this. Maybe this would be helpful but the allocate() function was the most confusing to me, that's why I picked that one.

fparain

LGTM.
Thank you for this cleanup.

coleenp · 2025-06-09T18:31:36Z

Thanks for reviewing Stefan and Frederic.
/integrate

openjdk · 2025-06-09T18:33:01Z

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

156187a: 8356978: Convert unicode sequences in Java source code to UTF-8
a377773: 8358617: java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java fails with 403 due to system proxies
cae1fd3: 8357632: CDS test failures on static JDK
... and 12 more: https://git.openjdk.org/jdk/compare/b2e7cda6a0bd21fa3c4ffe2a67da4953f1ca3f1f...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-06-09T18:33:11Z

@coleenp Pushed as commit eb256de.

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

openjdk bot added graal [email protected] hotspot [email protected] labels Jun 2, 2025

coleenp changed the title ~~8358326: Use oopFactory array allocation for jni~~ 8358326: Use oopFactory array allocation Jun 3, 2025

8358326: Use oopFactory array allocation

2da2b11

coleenp force-pushed the jni-array branch from 0cd49b1 to 2da2b11 Compare June 6, 2025 10:54

coleenp marked this pull request as ready for review June 6, 2025 12:02

openjdk bot added the rfr Pull request is ready for review label Jun 6, 2025

stefank approved these changes Jun 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 9, 2025

Sort our friends.

2ccaf42

openjdk bot removed the ready Pull request is ready to be integrated label Jun 9, 2025

fparain approved these changes Jun 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 9, 2025

openjdk bot added the integrated Pull request has been integrated label Jun 9, 2025

openjdk bot closed this Jun 9, 2025

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

coleenp deleted the jni-array branch June 12, 2025 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8358326: Use oopFactory array allocation #25590

8358326: Use oopFactory array allocation #25590

Uh oh!

coleenp commented Jun 2, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jun 2, 2025

Uh oh!

openjdk bot commented Jun 2, 2025 •

edited

Loading

Uh oh!

openjdk bot commented Jun 2, 2025

Uh oh!

mlbridge bot commented Jun 6, 2025 •

edited

Loading

Uh oh!

stefank left a comment

Uh oh!

stefank Jun 9, 2025 •

edited

Loading

Uh oh!

coleenp Jun 9, 2025

Uh oh!

stefank Jun 9, 2025

Uh oh!

coleenp Jun 9, 2025 •

edited

Loading

Uh oh!

stefank Jun 9, 2025

Uh oh!

coleenp Jun 9, 2025

Uh oh!

fparain left a comment

Uh oh!

coleenp commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

Uh oh!

8358326: Use oopFactory array allocation #25590

8358326: Use oopFactory array allocation #25590

Uh oh!

Conversation

coleenp commented Jun 2, 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 Jun 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Jun 2, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

stefank Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

stefank Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefank Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

fparain left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

openjdk bot commented Jun 9, 2025

Uh oh!

Uh oh!

coleenp commented Jun 2, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 2, 2025 •

edited

Loading

mlbridge bot commented Jun 6, 2025 •

edited

Loading

stefank Jun 9, 2025 •

edited

Loading

coleenp Jun 9, 2025 •

edited

Loading