8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types

[cl4es]

This PR makes ClassDesc.ofDescriptor return the shared constant for primitive descriptor strings ("I" etc..), and leverages this further by refactoring MethodTypeDescImpl.ofDescriptor to avoid the intermediate substring for primitives.

Microbenchmarks results look good with expected speedups and allocation reductions any time a primitive is part of the descriptor string, and a non-significant cost otherwise:

Name                                                             (descString) Cnt     Base     Error       Test     Error   Unit  Change
ClassDescFactories.ofDescriptor                            Ljava/lang/Object;   6   13,941 ±   1,643     14,071 ±   1,333  ns/op   0,99x (p = 0,681 )
  :gc.alloc.rate.norm                                                               16,000 ±   0,000     16,000 ±   0,000   B/op   1,00x (p = 0,617 )
ClassDescFactories.ofDescriptor                                             V   6    9,212 ±   1,045      1,405 ±   0,049  ns/op   6,55x (p = 0,000*)
  :gc.alloc.rate.norm                                                               48,000 ±   0,000      0,000 ±   0,000   B/op   0,00x (p = 0,000*)
ClassDescFactories.ofDescriptor                                             I   6    9,009 ±   0,035      1,431 ±   0,192  ns/op   6,30x (p = 0,000*)
  :gc.alloc.rate.norm                                                               48,000 ±   0,000      0,000 ±   0,000   B/op   0,00x (p = 0,000*)
MethodTypeDescFactories.ofDescriptor  (Ljava/lang/Object;Ljava/lang/String;)I   6  182,050 ±   4,333    141,644 ±   2,685  ns/op   1,29x (p = 0,000*)
  :gc.alloc.rate.norm                                                              360,001 ±   0,000    264,001 ±   0,000   B/op   0,73x (p = 0,000*)
MethodTypeDescFactories.ofDescriptor                                      ()V   6   17,169 ±   2,008      9,915 ±   0,018  ns/op   1,73x (p = 0,000*)
  :gc.alloc.rate.norm                                                              120,000 ±   0,000    104,000 ±   0,000   B/op   0,87x (p = 0,000*)
MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List;   6  270,372 ±   3,624    217,050 ±   3,170  ns/op   1,25x (p = 0,000*)
  :gc.alloc.rate.norm                                                              520,002 ±   0,000    328,001 ±   0,000   B/op   0,63x (p = 0,000*)
MethodTypeDescFactories.ofDescriptor                    ()[Ljava/lang/String;   6   35,036 ±   0,351     36,010 ±   3,882  ns/op   0,97x (p = 0,146 )
  :gc.alloc.rate.norm                                                              104,000 ±   0,000    104,000 ±   0,000   B/op   1,00x (p = 0,140 )
  * = significant

---

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-8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types (Enhancement - P4)

Reviewers

• Jorn Vernee (@JornVernee - Reviewer)
• Chen Liang (@liach - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18971

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back redestad! 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]

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

8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types

Reviewed-by: jvernee, 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 18 new commits pushed to the master branch:

• 5e2ced4: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases
• 2b7176a: 8330625: Compilation memory statistic: prevent tearing of the final report
• e4ed7ce: 8331063: Some HttpClient tests don't report leaks
• cfd19f0: 8329850: [AIX] Allow loading of different members of same shared library archive
• ffd850f: 8309881: Qualified name of a type element depends on its origin (source vs class)
• 377f2e5: 8329862: libjli GetApplicationHome cleanups and enhance jli tracing
• 006f090: 8331150: RISC-V: Fix "bad AD file" bug
• a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
• ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
• b3bcc49: 8330181: Move PcDesc cache from nmethod header
• ... and 8 more: https://git.openjdk.org/jdk/compare/b1b953c427b9562f050760ab3c2d0ae09aeec3e5...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]

@cl4es 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.

[mlbridge]

Webrevs

• 02: Full - Incremental (6a16a577)
• 01: Full - Incremental (257dc547)
• 00: Full (96aa846b)

[JornVernee]

Looks like your MethodTypeDescFactories benchmark is missing form the PR?

Does removing the explicit null checks make that much difference for performance? They are kind of nice for clarity.

Also, looks like the copyright year still needs to be updated on some of the files.

[cl4es]

Looks like your MethodTypeDescFactories benchmark is missing form the PR?

Pre-existing.

Does removing the explicit null checks make that much difference for performance? They are kind of nice for clarity.

It helps startup at least. The redundant array depth check mattered a bit for peak performance, but not much. I don't have a strong opinion really, just find them a bit superfluous when there's a dereference on the next line.

Also, looks like the copyright year still needs to be updated on some of the files.

Fixed, along with using EMPTY_CLASSDESC in the toArray call.

[JornVernee]

Does removing the explicit null checks make that much difference for performance? They are kind of nice for clarity.

It helps startup at least. The redundant array depth check mattered a bit for peak performance, but not much. I don't have a strong opinion really, just find them a bit superfluous when there's a dereference on the next line.

Sure, I'm mostly talking about cases where the value is passed to another method.

[JornVernee]

LGTM

[liach]

Primitive class desc sharing should be really helpful, especially considering that they are represented as condy and their equals is thus quite inefficient.

[cl4es]

Primitive class desc sharing should be really helpful, especially considering that they are represented as condy and their equals is thus quite inefficient.

Ok. We could consider overriding equals in PrimitiveClassDescImpl if this is an issue (in a follow-up). With guarantees that they are unique we could even have such a method be implemented as a reference equality check.

[cl4es]

/integrate

[openjdk]

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

• 5e2ced4: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases
• 2b7176a: 8330625: Compilation memory statistic: prevent tearing of the final report
• e4ed7ce: 8331063: Some HttpClient tests don't report leaks
• cfd19f0: 8329850: [AIX] Allow loading of different members of same shared library archive
• ffd850f: 8309881: Qualified name of a type element depends on its origin (source vs class)
• 377f2e5: 8329862: libjli GetApplicationHome cleanups and enhance jli tracing
• 006f090: 8331150: RISC-V: Fix "bad AD file" bug
• a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
• ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
• b3bcc49: 8330181: Move PcDesc cache from nmethod header
• ... and 8 more: https://git.openjdk.org/jdk/compare/b1b953c427b9562f050760ab3c2d0ae09aeec3e5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@cl4es Pushed as commit 8bbd725.

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

[liach]

For the equals remark, it's that DynamicConstantDesc's equals is final so subclasses cannot provide a faster implementation. But that's another topic.

[cl4es]

For the equals remark, it's that DynamicConstantDesc's equals is final so subclasses cannot provide a faster implementation. But that's another topic.

Hmm, that is unfortunate, yes.

[mlchung]

As this method is named as primitiveClassDescriptor, I expect this should either throw or return null for OBJECT and even VOID wrapper. Or should be named "classDescriptor".

[cl4es]

While I agree it may seem non-sensical to define CD_Object for Object, there's precedent in that Wrapper.primitiveType is Object.class - so for consistency I wired it up the same way. For the usage patterns introduced with this patch we'll never call primitiveClassDescriptor() on Wrapper.OBJECT, though - and Wrapper.forPrimitiveType will throw an IAE if you try - so we can always revisit this.

void.class is the primitive type for Void.class, allowed as a return type and recognized by Wrapper.forPrimitiveType. There are checks in for example ConstantUtils.parseMethodDescriptor which disallows void.class anywhere but as a return type.

[mlchung]

I know it'll never call primitiveClassDescriptor() besides primitive types. While void.class.isPrimitive() returns true, void is not a primitive type and called out explicitly in the spec.

My point is about the behavior does not match the method name. It may be better to rename it to classDescriptor instead. It's okay to clean up in your other performance fix (perhaps #18945).

[cl4es]

Updated #18945, will try ignoring the pre-existing Wrapper.primitiveType inconsistencies.