8331114: Further improve performance of MethodTypeDesc::descriptorString

[cl4es]

When analyzing (startup) performance of the Classfile API I found this opportunity to further improve MethodTypeDescImpl::descriptorString.

Performance improves across the board in existing microbenchmarks:

Name                                                                 (descString) Cnt   Base   Error    Test   Error  Unit  Change
MethodTypeDescFactories.descriptorString  (Ljava/lang/Object;Ljava/lang/String;)I   6 55,179 ± 2,027  32,920 ± 1,189 ns/op   1,68x (p = 0,000*)
MethodTypeDescFactories.descriptorString                                      ()V   6 17,689 ± 1,871  11,060 ± 0,331 ns/op   1,60x (p = 0,000*)
MethodTypeDescFactories.descriptorString ([IJLjava/lang/String;Z)Ljava/util/List;   6 86,627 ± 1,646  41,035 ± 0,636 ns/op   2,11x (p = 0,000*)
MethodTypeDescFactories.descriptorString                    ()[Ljava/lang/String;   6 18,305 ± 1,974  13,110 ± 0,089 ns/op   1,40x (p = 0,000*)
  * = significant

The improvement is even more pronounced when running with -Xint, which is relevant for reducing startup overheads of early ClassFile API use:

Name                                                                 (descString) Cnt     Base      Error      Test     Error  Unit  Change
MethodTypeDescFactories.descriptorString  (Ljava/lang/Object;Ljava/lang/String;)I   6 5122,061 ±   81,335  2626,481 ± 101,466 ns/op   1,95x (p = 0,000*)
MethodTypeDescFactories.descriptorString                                      ()V   6 3481,316 ±  258,904  1489,267 ±  15,506 ns/op   2,34x (p = 0,000*)
MethodTypeDescFactories.descriptorString ([IJLjava/lang/String;Z)Ljava/util/List;   6 7741,081 ± 1628,244  3281,778 ±  41,892 ns/op   2,36x (p = 0,000*)
MethodTypeDescFactories.descriptorString                    ()[Ljava/lang/String;   6 3677,803 ±   63,432  1495,291 ±   8,995 ns/op   2,46x (p = 0,000*)
  * = significant

I also applied similar approach to MethodTypeDesc::displayDescriptor: while not performance sensitive I think these are so inter-related that it makes sense to implement them in a similar fashion.

---

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-8331114: Further improve performance of MethodTypeDesc::descriptorString (Enhancement - P4)

Reviewers

• Mandy Chung (@mlchung - Reviewer)
• Chen Liang (@liach - Author) ⚠️ Review applies to 7622f8f4

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18945

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18945.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:

8331114: Further improve performance of MethodTypeDesc::descriptorString

Reviewed-by: mchung, 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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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

• 04: Full - Incremental (fff380df)
• 03: Full - Incremental (6ff505f6)
• 02: Full - Incremental (15c8d39c)
• 01: Full - Incremental (2979847c)
• 00: Full (06947305)

[liach]

Do we have any research on the average length of method descriptor strings? I wonder if manual pre-allocation works better (iterating all descriptor strings, allocate the sum of their sizes plus 2 (for parentheses), as descriptor strings won't be re-calculated after initial allocation.)
Especially in case of user code, as many user classes have very long package names that can easily make the string much longer than the 24-char default.

[cl4es]

Do we have any research on the average length of method descriptor strings? I wonder if manual pre-allocation works better (iterating all descriptor strings, allocate the sum of their sizes plus 2 (for parentheses), as descriptor strings won't be re-calculated after initial allocation.) Especially in case of user code, as many user classes have very long package names that can easily make the string much longer than the 24-char default.

I don't know of any systematic research on this, but in the code generators we have in the JDK short method signatures outweigh more complex ones by far. So if we can improve for small descriptors without regressing large ones then that's a pretty good win.

I've done some micro-benchmarking on longer descriptors and there's still a gain from the proposed patch:

MethodTypeDescFactories.ofDesc -p descString=(Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories;Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories;Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories;)Lorg/openjdk/bench/java/lang/constant/MethodTypeDescFactories;

Name                                 Cnt    Base    Error     Test   Error  Unit  Change
MethodTypeDescFactories.ofDescriptor   6 442,112 ± 16,175  423,926 ± 6,245 ns/op   1,04x (p = 0,000*)
  * = significant

I'd be happy to add a variant that stresses larger descriptors.

[liach]

What if we replace 24 with a precalculated value:

int size = 2 + returnType().descriptorString().length();
for (var param : argTypes)
    size += param.descriptorString().length();

(Would be even better if we can just trust the internal array to avoid copy allocation)

[cl4es]

Correction: I ran the wrong benchmark for that long descriptor test! 🤦🏽 On the descriptorString benchmark there is a regression with my patch on longer descriptors, -30% on that example. Since the strings are all readily available pre-calculating the size is cheap enough to make sense. This makes the patch more or less neutral on large descriptors, extends the gain on few-arg methods, but reduces the win on cases where there are no args (due an extra branch et.c.). On balance this seems like an improvement:

Name                                                                 (descString) Cnt    Base   Error    Test    Error  Unit  Change
MethodTypeDescFactories.descriptorString  (Ljava/lang/Object;Ljava/lang/String;)I   6  43,180 ± 0,575   30,547 ± 0,387 ns/op   1,41x (p = 0,000*)
MethodTypeDescFactories.descriptorString                                      ()V   6  20,717 ± 0,128   17,233 ± 0,242 ns/op   1,20x (p = 0,000*)
MethodTypeDescFactories.descriptorString ([IJLjava/lang/String;Z)Ljava/util/List;   6  89,834 ± 1,951   40,149 ± 0,326 ns/op   2,24x (p = 0,000*)
MethodTypeDescFactories.descriptorString                    ()[Ljava/lang/String;   6  21,438 ± 0,490   15,937 ± 0,263 ns/op   1,35x (p = 0,000*)
MethodTypeDescFactories.descriptorString                                 (..IIJ)V   6  52,429 ± 0,117   49,396 ± 0,153 ns/op   1,06x (p = 0,000*)
MethodTypeDescFactories.descriptorString                 (.....................).   6 178,205 ± 0,518  176,856 ± 5,577 ns/op   1,01x (p = 0,158 )
  * = significant

. is illegal in descriptor strings I used this as wildcard to be replaced with the descriptor string for the benchmark class as a ways to produce very large descriptors.

Since this strays a bit from what's doable in MethodTypeDesc::displayDescriptor I'm reverting those changes and focus this PR on the relevant piece of machinery.

[liach]

For the precise-length approach, do you have the results? I guess if we can avoid copying the stringbuilder array, we can make this even faster.

[cl4es]

For the precise-length approach, do you have the results? I guess if we can avoid copying the stringbuilder array, we can make this even faster.

Not sure what you mean - the latest performance numbers I posted are for the precise length version (15c8d39) if that was unclear.

[mlchung]

I am glad that this only focuses on the descriptor string as MethodTypeDesc::displayDescriptor is typically used for debugging and exception message and not performance-sensitive.

[cl4es]

I am glad that this only focuses on the descriptor string as MethodTypeDesc::displayDescriptor is typically used for debugging and exception message and not performance-sensitive.

Yes, which is why I'd really like to desugar it: this is the method where I tripped on a bootstrap circularity when print-debugging an issue in the lambda code generator. It's no fun at all hitting bootstrapping issues when calling innocuous methods in java.lang.*

Obviously the performance of it is not important - I was just stringing it along for consistency.

[liach]

You can update the copyright of this file if you don't mind.

[mlchung]

Thanks for following up the feedback for 8331187.

[cl4es]

Thanks!

/integrate

[openjdk]

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

• e3eb652: 8296543: Update exception documentation for ExecutorService.invokeAll overriders as required
• aa2edd4: 8330686: Fix typos in the DatabaseMetaData javadoc

Your commit was automatically rebased without conflicts.

[openjdk]

@cl4es Pushed as commit a078b5e.

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