8266622: Optimize Class.descriptorString() and Class.getCanonicalName0() #3903
Conversation
|
👋 Welcome back stsypanov! A progress list of the required criteria for merging this PR into |
|
@stsypanov The following label will be automatically applied to this pull request:
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. |
Webrevs
|
|
Hi Sergei, as an alternative to: or for example: as an alternative for: and see how it compares? Regards, Peter |
|
@plevart hi, to my surprise |
Yeah, it seems JIT does a very good job with StringBuilder in this form while the overheads of redundant byte[] copying doesn't show yet at string lengths typical for class names. |
|
Together with #3627 this allows to reduce minimalistic Spring Boot application start-up time from 653 to 645 milliseconds and memory consumprion from 43804 to 43668 kB. |
| .append('L') | ||
| .append(name.substring(0, index).replace('.', '/')) | ||
| .append('.') | ||
| .append(name.substring(index + 1)) |
There was a problem hiding this comment.
.append(name, index + 1, name.length()) might be a small win here, but it might be hard to benchmark this branch since it's only for hidden classes.
There was a problem hiding this comment.
Indeed, I've measured the case for hidden classes with benchmark
@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class HiddenClassBenchmark {
private Class<?> hiddenClass;
@Setup
public void setUp() throws Exception {
byte[] bytes = getClassWriter().toByteArray();
hiddenClass = MethodHandles
.lookup()
.defineHiddenClass(bytes, true, NESTMATE)
.lookupClass();
if (hiddenClass.isHidden()) {
return;
}
throw new RuntimeException();
}
@Benchmark
public String descriptorString() {
return hiddenClass.descriptorString();
}
private static ClassWriter getClassWriter() {
ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_MAXS);
var name = HiddenClassDemo.class.getName().replace('.', '/');
cw.visit(V1_6, ACC_PUBLIC + ACC_SUPER, name, null, "java/lang/Object", null);
cw.visitEnd();
return cw;
}
private static class HiddenClassDemo {
}
}and got those results:
jdk 16
Benchmark Mode Cnt Score Error Units
HiddenClassBenchmark.descriptorString avgt 100 112.591 ± 1.320 ns/op
HiddenClassBenchmark.descriptorString:·gc.alloc.rate.norm avgt 100 600.045 ± 0.001 B/op
patched
Benchmark Mode Cnt Score Error Units
HiddenClassBenchmark.descriptorString avgt 100 85.958 ± 0.561 ns/op
HiddenClassBenchmark.descriptorString:·gc.alloc.rate.norm avgt 100 448.034 ± 0.001 B/op
patched without substring
Benchmark Mode Cnt Score Error Units
HiddenClassBenchmark.descriptorString avgt 100 76.580 ± 0.587 ns/op
HiddenClassBenchmark.descriptorString:·gc.alloc.rate.norm avgt 100 432.031 ± 0.001 B/op
|
@stsypanov 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: 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 115 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@cl4es) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
How do you run this benchmark? Something like |
Yes, I use SingleShotTime as the mode and 400 forks |
|
/integrate |
|
@stsypanov |
|
/integrate |
|
@stsypanov |
|
/sponsor |
openjdk
bot
added
integrated
|
@cl4es @stsypanov Since your change was applied there have been 124 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit ebcf399. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hello, from discussion in #3464 and #2212 it appears, that in
j.l.Classexpressions likeare not compiled into invokedynamic-based code, but into one using
StringBuilder.This happens due to some bootstraping issues. Currently the bytecode for the last (most often used) branch of
Class.descriptorString()looks likeHere the
StringBuilderis created with default constructor and then expands if necessary while appending.This can be improved by manually allocating
StringBuilderof exact size. The benchmark demonstrates measurable improvement:Same can be done also for Class.isHidden() branch in Class.descriptorString() and for Class.getCanonicalName0()
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3903/head:pull/3903$ git checkout pull/3903Update a local copy of the PR:
$ git checkout pull/3903$ git pull https://git.openjdk.java.net/jdk pull/3903/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3903View PR using the GUI difftool:
$ git pr show -t 3903Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3903.diff