8338544: Dedicated Array class descriptor implementation #20665

liach · 2024-08-21T20:25:07Z

@cl4es discovered that Stack Map generation in ClassFile API uses componentType and arrayType for aaload aastore instructions, which are currently quite slow. We can split out array class descriptors from class or interfaces to support faster arrayType and componentType operations.

Tentative, as I currently have no way to measure the actual impact of this patch on the startup performance; however, this made the ClassDesc implementations much cleaner.

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
Change requires CSR request JDK-8340963 to be approved

Issues

JDK-8338544: Dedicated Array class descriptor implementation (Sub-task - P4)
JDK-8340963: Make some ClassDesc methods no longer default (CSR)

Reviewers

Claes Redestad (@cl4es - Reviewer)
Jorn Vernee (@JornVernee - Reviewer) Review applies to a1d14186
Mandy Chung (@mlchung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20665

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

Using diff file

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

Webrev

Link to Webrev Comment

…array-cd

bridgekeeper · 2024-08-21T20:25:49Z

👋 Welcome back liach! 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-08-21T20:25:56Z

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

8338544: Dedicated Array class descriptor implementation

Reviewed-by: redestad, mchung, jvernee

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

98403b7: 8342854: [JVMCI] Block secondary thread reporting a JVMCI fatal error
9a7a850: 8341939: SigningOptionsTest fails without Xcode with command line developer tools after JDK-8341443
de92fe3: 8233451: (fs) Files.newInputStream() cannot be used with character special files
002de86: 8342673: Test serviceability/jvmti/events/NotifyFramePopStressTest/NotifyFramePopStressTest.java failed: waited too long for notify
a21c558: 8342863: Use pattern matching for instanceof in equals methods of wrapper classes
e64f079: 8342582: user.region for formatting number no longer works for 21.0.5
426da4b: 8341975: Unable to set encoding for IO.println, IO.print and IO.readln
a522d21: 8342858: Make target mac-jdk-bundle fails on chmod command
afb62f7: 8342683: Use non-short forward jump when passing stop()
964d8d2: 8340445: [PPC64] Wrong ConditionRegister used in ppc64.ad: flagsRegCR0 cr1
... and 7 more: https://git.openjdk.org/jdk/compare/d6eddcdaf92f2352266ba519608879141997cd63...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-08-21T20:26:19Z

@liach The following label will be automatically applied to this pull request:

core-libs

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

ExE-Boss

Apparently, changing method default‑ness requires a CSR (even for fully sealed hierarchies), but I can’t find the PR where I was informed of this.

Using return this instanceof <type>ClassDescImpl might allow C2 to better avoid megamorphic virtual invocations (see GH‑17488), but this needs to be benchmarked:

src/java.base/share/classes/java/lang/constant/ClassDesc.java

…array-cd

liach · 2024-08-27T17:26:22Z

I know this requires a CSR; it's just not created due to this still being a draft.

The main purpose of this is still for speeding up stack map generation; the results are not confirmed yet, so this patch is on hold.

For megamorphic call site thing, the regular workload would be like:

if (c.isArray()) doArrayStuff(c);
if (c.isClassOrInterface()) doClassOrInterfaceStuff(c);

I think polymorphism is fine here, as after the check we usually go straight to perform type-specific operations like componentType(); or sometimes a method expects the input ClassDesc to be always primitive/class or interface/array, which won't lead to profile pollutions. This concern might be more valid for descriptorString I suppose.

…array-cd

liach · 2024-09-25T18:48:19Z

Confirmed this is performance-wise neutral for startup. Ready for review.

mlbridge · 2024-09-25T18:54:01Z

Webrevs

cl4es

LGTM. Always a bit queasy to add a third to a nice pair of classes, but if this ever shows up as a problem on benchmarks we can revisit and think of alternatives (such as adding a rank field in each impl).

src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java

JornVernee

Nice cleanup! I'm just not really sure about the new subtype relationship (see inline comment)

src/java.base/share/classes/jdk/internal/constant/ArrayClassDescImpl.java

JornVernee · 2024-10-04T17:48:51Z

src/java.base/share/classes/jdk/internal/constant/ArrayClassDescImpl.java

+        // Class.forName is slow on class or interface arrays
+        Class<?> clazz = element.resolveConstantDesc(lookup);
+        for (int i = 0; i < rank; i++)
+            clazz = clazz.arrayType();


Looking at the implementation of arrayType, it reflectively creates an array and then returns its class. Just makes me think we need a better we to look up an array class directly in the JDK :D

Indeed; and it's more alarming that Class.forName("[[[[[Ljava.lang.Object;") is slower than Object.class.arrayType().arrayType().arrayType().arrayType().arrayType().

src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java

src/java.base/share/classes/jdk/internal/constant/ReferenceClassDescImpl.java

…array-cd

liach · 2024-10-08T01:24:51Z

@JornVernee I have updated the implementation class name and the factories for clarity.

cl4es

Looks reasonable to me.

An alternative could be to put an int rank in both PrimitiveClassDescImpl and ReferenceClassDescImpl. Would lead to slightly gnarlier code in places, but keep the number of classes down. Worth considering if any throughput benchmark sees fallout from going from 2 to 3 types.

The internalNameToDesc changes could perhaps better have been done in a separate PR but no point teasing it apart now.

mlchung

Looks good with minor suggestion. I agree having a separate implementation class for array type and class/interface allows a cleaner subclass implementation.

src/java.base/share/classes/jdk/internal/constant/ArrayClassDescImpl.java

liach · 2024-10-08T19:28:54Z

Thanks for the reviews; can anyone review the associated CSR at https://bugs.openjdk.org/browse/JDK-8340963 as well?

src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java

openjdk · 2024-10-18T20:40:51Z

@liach this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/array-cd
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

…array-cd

src/java.base/share/classes/java/lang/constant/ClassDesc.java

…array-cd

src/java.base/share/classes/java/lang/constant/ClassDesc.java

Turns out ArrayClassDesc::displayName was incorrect. Added a test.

liach · 2024-10-23T06:11:15Z

Just noticed that the ArrayClassDesc::displayName was buggy - existing tests had no coverage and I have added test coverage for displayName and a few other missed methods.

cl4es

Updates since last review looks good

mlchung

Looks good.

liach · 2024-10-24T02:37:56Z

Thanks for the reviews!

/integrate

openjdk · 2024-10-24T02:40:08Z

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

158b93d: 8335912: Add an operation mode to the jar command when extracting to not overwriting existing files
28d23ad: 8340177: Malformed system classes loaded by bootloader crash the JVM in product builds
98403b7: 8342854: [JVMCI] Block secondary thread reporting a JVMCI fatal error
9a7a850: 8341939: SigningOptionsTest fails without Xcode with command line developer tools after JDK-8341443
de92fe3: 8233451: (fs) Files.newInputStream() cannot be used with character special files
002de86: 8342673: Test serviceability/jvmti/events/NotifyFramePopStressTest/NotifyFramePopStressTest.java failed: waited too long for notify
a21c558: 8342863: Use pattern matching for instanceof in equals methods of wrapper classes
e64f079: 8342582: user.region for formatting number no longer works for 21.0.5
426da4b: 8341975: Unable to set encoding for IO.println, IO.print and IO.readln
a522d21: 8342858: Make target mac-jdk-bundle fails on chmod command
... and 9 more: https://git.openjdk.org/jdk/compare/d6eddcdaf92f2352266ba519608879141997cd63...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-24T02:40:16Z

@liach Pushed as commit 25c2f48.

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

JornVernee

Latest version looks good.

P.S. Oh, looks like the tab I had open for this PR wasn't updated. Didn't see it was already merged. All good!

liach added 4 commits August 19, 2024 15:13

8338544: Dedicated Array class descriptor implementation

0b51431

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

51e9945

…array-cd

Redundant import

d1f1729

Compile error

a424ada

openjdk bot added the core-libs core-libs-dev@openjdk.org label Aug 21, 2024

ExE-Boss reviewed Aug 27, 2024

View reviewed changes

src/java.base/share/classes/java/lang/constant/ClassDesc.java Show resolved Hide resolved

src/java.base/share/classes/java/lang/constant/ClassDesc.java Show resolved Hide resolved

src/java.base/share/classes/java/lang/constant/ClassDesc.java Show resolved Hide resolved

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

9eb2868

…array-cd

liach added 2 commits September 24, 2024 15:24

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

c648ce5

…array-cd

Cleanup after merge

81c2fa0

liach marked this pull request as ready for review September 25, 2024 18:48

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Sep 25, 2024

cl4es approved these changes Sep 25, 2024

View reviewed changes

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

JornVernee reviewed Oct 4, 2024

View reviewed changes

liach added 2 commits October 7, 2024 15:23

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

f13588e

…array-cd

class or interface descriptor renamed for clarity

a1d1418

liach requested review from JornVernee and cl4es October 8, 2024 01:35

cl4es approved these changes Oct 8, 2024

View reviewed changes

mlchung approved these changes Oct 8, 2024

View reviewed changes

src/java.base/share/classes/jdk/internal/constant/ArrayClassDescImpl.java Outdated Show resolved Hide resolved

src/java.base/share/classes/jdk/internal/constant/ArrayClassDescImpl.java Outdated Show resolved Hide resolved

mlchung suggestions: elementType, improve utility methods

f6964d1

mlchung reviewed Oct 8, 2024

View reviewed changes

src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java Outdated Show resolved Hide resolved

liach requested a review from JornVernee October 11, 2024 14:45

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Oct 18, 2024

liach added 4 commits October 21, 2024 10:13

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

c0218cb

…array-cd

object array desc constant

28e8f29

Merge

b0e8d2e

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

62deed8

…array-cd

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Oct 22, 2024

Relax displayName spec

4acec98

openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 22, 2024

mlchung reviewed Oct 22, 2024

View reviewed changes

src/java.base/share/classes/java/lang/constant/ClassDesc.java Show resolved Hide resolved

openjdk bot added the csr Pull request needs approved CSR before integration label Oct 23, 2024

liach added 2 commits October 22, 2024 19:50

Restore the impl spec to the API spec

c031b45

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

bf9ff94

…array-cd

mlchung reviewed Oct 23, 2024

View reviewed changes

src/java.base/share/classes/java/lang/constant/ClassDesc.java Show resolved Hide resolved

Fix displayName

f6bfa12

Turns out ArrayClassDesc::displayName was incorrect. Added a test.

cl4es approved these changes Oct 23, 2024

View reviewed changes

mlchung approved these changes Oct 23, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Oct 23, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 24, 2024

openjdk bot closed this Oct 24, 2024

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

JornVernee reviewed Oct 24, 2024

View reviewed changes

liach mentioned this pull request Nov 1, 2024

8343437: ClassDesc.of incorrectly permitting empty names #21830

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8338544: Dedicated Array class descriptor implementation #20665

8338544: Dedicated Array class descriptor implementation #20665

liach commented Aug 21, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 21, 2024

openjdk bot commented Aug 21, 2024 •

edited

Loading

openjdk bot commented Aug 21, 2024

ExE-Boss left a comment

liach commented Aug 27, 2024

liach commented Sep 25, 2024

mlbridge bot commented Sep 25, 2024 •

edited

Loading

cl4es left a comment

JornVernee left a comment •

edited

Loading

JornVernee Oct 4, 2024

liach Oct 4, 2024

liach commented Oct 8, 2024

cl4es left a comment

mlchung left a comment

liach commented Oct 8, 2024

openjdk bot commented Oct 18, 2024

liach commented Oct 23, 2024

cl4es left a comment

mlchung left a comment

liach commented Oct 24, 2024

openjdk bot commented Oct 24, 2024

openjdk bot commented Oct 24, 2024

JornVernee left a comment •

edited

Loading

8338544: Dedicated Array class descriptor implementation #20665

8338544: Dedicated Array class descriptor implementation #20665

Conversation

liach commented Aug 21, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 21, 2024

openjdk bot commented Aug 21, 2024 • edited Loading

openjdk bot commented Aug 21, 2024

ExE-Boss left a comment

Choose a reason for hiding this comment

liach commented Aug 27, 2024

liach commented Sep 25, 2024

mlbridge bot commented Sep 25, 2024 • edited Loading

Webrevs

cl4es left a comment

Choose a reason for hiding this comment

JornVernee left a comment • edited Loading

Choose a reason for hiding this comment

JornVernee Oct 4, 2024

Choose a reason for hiding this comment

liach Oct 4, 2024

Choose a reason for hiding this comment

liach commented Oct 8, 2024

cl4es left a comment

Choose a reason for hiding this comment

mlchung left a comment

Choose a reason for hiding this comment

liach commented Oct 8, 2024

openjdk bot commented Oct 18, 2024

liach commented Oct 23, 2024

cl4es left a comment

Choose a reason for hiding this comment

mlchung left a comment

Choose a reason for hiding this comment

liach commented Oct 24, 2024

openjdk bot commented Oct 24, 2024

openjdk bot commented Oct 24, 2024

JornVernee left a comment • edited Loading

Choose a reason for hiding this comment

liach commented Aug 21, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 21, 2024 •

edited

Loading

mlbridge bot commented Sep 25, 2024 •

edited

Loading

JornVernee left a comment •

edited

Loading

JornVernee left a comment •

edited

Loading