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) 🔄 Re-review required (review applies to a1d14186)
Jorn Vernee (@JornVernee - Reviewer) 🔄 Re-review required (review applies to a1d14186)
Mandy Chung (@mlchung - Reviewer) 🔄 Re-review required (review applies to a1d14186)

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

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).

cl4es · 2024-09-25T19:53:13Z

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

-    private static final ClassDesc CD_Object_array  = ReferenceClassDescImpl.ofValidated("[Ljava/lang/Object;");
-    private static final ClassDesc CD_MethodHandle_array = ReferenceClassDescImpl.ofValidated("[Ljava/lang/invoke/MethodHandle;");
-    private static final ClassDesc CD_MethodHandle_array2 = ReferenceClassDescImpl.ofValidated("[[Ljava/lang/invoke/MethodHandle;");
+    private static final ClassDesc CD_Object_array  = CD_Object.arrayType();


I guess CD_Object.arrayType() shows up often enough now - even once in java.lang.constant.ConstantDescs - that we might as well pin it down as a constant somewhere (ConstantDescs is a candidate location, but that will take a CSR).

This patch already has a CSR for trivial signature changes. The real difficulty lies in how we should name our new array class descriptors, Object_array or ObjectArray or what else?

That said, can you leave a quick review on CSR https://bugs.openjdk.org/browse/JDK-8340963 too?

I think I will do this in another patch that adds it to ConstantDescs - there's a place in ConstantDescs that could have used it, but if I put it in ConstantUtils I fear we are at risk of circular initialization dependencies.

JornVernee

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

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

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

+    public String descriptorString() {
+        var desc = cachedDescriptorString;
+        if (desc != null)
+            return desc;
+
+        return cachedDescriptorString = computeDescriptor();
+    }
+
+    private String computeDescriptor() {
+        var componentDesc = element.descriptorString();
+        StringBuilder sb = new StringBuilder(rank + componentDesc.length());
+        sb.repeat('[', rank);
+        sb.append(componentDesc);
+        return sb.toString();
+    }


Is there really that much benefit in lazily computing the descriptor? ReferenceClassDescImpl doesn't do this either... Maybe we can keep things simple and initialize the descriptor in the constructor?

This laziness is actually the main reason I had this implementation: In stack map generation, we need to compute the array descriptors of many types yet not using them in the end; the string computation involved a lot of allocations, especially with frequent arrayType() and componentType() calls.

Ok, thanks. That's useful information.

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

mlchung · 2024-10-08T18:16:34Z

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

+        if (rank == MAX_ARRAY_TYPE_DESC_DIMENSIONS)
+            throw new IllegalStateException(
+                "Cannot create an array type descriptor with more than "
+                        + MAX_ARRAY_TYPE_DESC_DIMENSIONS + " dimensions");


Suggested change

if (rank == MAX_ARRAY_TYPE_DESC_DIMENSIONS)

throw new IllegalStateException(

"Cannot create an array type descriptor with more than "

+ MAX_ARRAY_TYPE_DESC_DIMENSIONS + " dimensions");

ConstantUtils.validateArrayDepth(rank + 1);

Unfortunately, this has to throw ISE while validateArrayDepth throws IAE.

I have extracted this to a new utility method, with a switch on the thrown exception type.

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?

mlchung · 2024-10-08T19:42:14Z

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

+    public static void ensureRankPositive(int rank) {
+        if (rank <= 0) {
+            throw new IllegalArgumentException("rank " + rank + " is not a positive value");
+        }
+    }


Suggest to inline the rank argument check in the caller method which makes the check explicit to the reader.

So like if (rank <= 0) throw ConstantUtils.rankNotPositive(rank); at individual use sites?

I meant no need to have a utility method. Just do this:

Suggested change

public static void ensureRankPositive(int rank) {

if (rank <= 0) {

throw new IllegalArgumentException("rank " + rank + " is not a positive value");

}

}

public static void validateArrayDepth(int rank) {

if (rank <= 0) {

throw new IllegalArgumentException("rank " + rank + " is not a positive value");

}

validateMaxArrayDepth(rank, true);

}

Same change in ArrayClassDescImpl::arrayType(int)

Done. The negative rank check is pulled out separately in ArrayClassDescImpl::arrayType(int).

mlchung · 2024-10-08T19:47:06Z

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

    }

    /**
     * Returns a human-readable name for the type described by this descriptor.
     *
     * @implSpec
-     * <p>The default implementation returns the simple name
+     * <p>The implementations return the simple name


Suggested change

* <p>The implementations return the simple name

* <p>The implementation returns the simple name

I think this can be singular as it's not required to have more than 1 implementation class.

Hmm, in fact, should we still have this @implSpec if we make this default?

I think this can be dropped as ClassDesc as well. It can say "This method returns....". See what CSR review would say.

JornVernee

Latest version looks good

…array-cd

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

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

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

mlchung suggestions: elementType, improve utility methods

f6964d1

mlchung reviewed Oct 8, 2024

View reviewed changes

JornVernee approved these changes Oct 8, 2024

View reviewed changes

liach added 2 commits October 11, 2024 09:19

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

ba51c8d

…array-cd

Mandy reviews

e65fa6a

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

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

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

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

cl4es Sep 25, 2024

liach Sep 25, 2024

liach Sep 25, 2024

liach Oct 8, 2024

JornVernee left a comment •

edited

Loading

JornVernee Oct 4, 2024

liach Oct 4, 2024

JornVernee Oct 4, 2024

JornVernee Oct 4, 2024

liach Oct 4, 2024

liach commented Oct 8, 2024

cl4es left a comment

mlchung left a comment

mlchung Oct 8, 2024

liach Oct 8, 2024

liach Oct 8, 2024

liach commented Oct 8, 2024

mlchung Oct 8, 2024

liach Oct 8, 2024

mlchung Oct 8, 2024 •

edited

Loading

liach Oct 11, 2024

mlchung Oct 8, 2024

liach Oct 8, 2024

mlchung Oct 8, 2024

JornVernee left a comment

openjdk bot commented Oct 18, 2024

	* <p>The implementations return the simple name
	* <p>The implementation returns the simple name

8338544: Dedicated Array class descriptor implementation #20665

Are you sure you want to change the base?

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

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

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

JornVernee left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlchung Oct 8, 2024 • edited Loading

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

JornVernee left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 18, 2024

liach commented Aug 21, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 25, 2024 •

edited

Loading

JornVernee left a comment •

edited

Loading

mlchung Oct 8, 2024 •

edited

Loading