8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available #3627
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. |
stsypanov
changed the title
| @@ -4348,7 +4348,7 @@ public String descriptorString() { | |||
| return Wrapper.forPrimitiveType(this).basicTypeString(); | |||
|
|
|||
| if (isArray()) { | |||
| return "[" + componentType.descriptorString(); | |||
| return "[".concat(componentType.descriptorString()); | |||
There was a problem hiding this comment.
This could be optimized further for multi-dimensional arrays:
if (isArray()) {
int arrayDepth = 1;
Class<?> ct = componentType;
while (ct.isArray()) {
arrayDepth++;
ct = ct.componentType;
}
return "[".repeat(arrayDepth).concat(ct.descriptorString());
There was a problem hiding this comment.
But isn't componentType.descriptorString() does this itself? Also multi-dimensional arrays are quite an infrequent usecase, aren't they?
There was a problem hiding this comment.
Yeah, it's just an optimization. Current code wouldbuild "[[..[[LFoo;" by recursively going down to Foo, build and return "LFoo;", then do "[" + "LFoo;", and so on. Infrequent enough that we can ignore it, sure, but since it's effectively reducing the algorithmic complexity from O(n*m) to O(n+m) I should at least mention it.
There was a problem hiding this comment.
I see that TestPrimitiveArrayCriticalWithBadParam constantly fails, so I'd probably revert this change
|
/issue 8266972 |
openjdk
bot
changed the title
|
@stsypanov The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated. |
Webrevs
|
|
Mailing list message from Michael Kroll on core-libs-dev: Hello, just being curious here: greets, Am 12. Mai 2021 15:04:21 MESZ schrieb "?????? ???????" <github.com+10835776+stsypanov at openjdk.java.net>: |
|
Mailing list message from Claes Redestad on core-libs-dev: Hi, I gave this some thought the other day when looking at one of Sergey's I'm no expert on javac but I think translating to String::concat is There are more convoluted solutions that might work if one squints (add It might help some if you're targeting java 8 or using javac as a That's why I think using String::concat on a case-by-case basis in those Hope this clarifies how I reason about this. /Claes On 2021-05-12 22:49, Michael Kroll wrote:
|
|
Mailing list message from Сергей Цыпанов on core-libs-dev:
I think the main reason is that String str = "smth is " + null; returns "smth is null" and with current implementation of String.concat() public String concat(String str) { Regards, |
|
Mailing list message from Michael Kroll on core-libs-dev: Okay, that makes sense. Greetings, Am 13. Mai 2021 00:45:13 MESZ schrieb Claes Redestad <claes.redestad at oracle.com>:
|
|
Spotted one more method to be improved: @State(Scope.Benchmark)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
public class ClassToStringBenchmark {
private final Class<?> clazzWithShortName = Object.class;
private final Class<?> interfaceWithShortName = Serializable.class;
private final Class<?> primitive = int.class;
@Benchmark
public String classToString() {
return clazzWithShortName.toString();
}
@Benchmark
public String interfaceToString() {
return interfaceWithShortName.toString();
}
@Benchmark
public String primitiveToString() {
return primitive.toString();
}
}giving |
|
|
|
|
@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 72 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 |
|
/integrate |
|
@stsypanov |
|
/integrate |
|
@stsypanov |
|
/sponsor |
|
Going to push as commit 6c4c48f.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
@cl4es @stsypanov Pushed as commit 6c4c48f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hello, from discussion in #3464 I've found out, that in a few of JDK core classes, e.g. in
j.l.Classexpressions likebaseName.replace('.', '/') + '/' + nameare not compiled intoinvokedynamic-based code, but into one usingStringBuilder. This happens due to some bootstraping issues.However the code like
can be improved via replacement of
+withString.concat()call.I've used this benchmark to measure impact:
and got those results
I think this patch is likely to improve reflection operations related to arrays.
If one finds this patch useful, then I'll create a ticket to track this. Also it'd be nice to have a list of classes, that are compiled in the same way as
j.l.Class(i.e. have no access toinvokedynamic) so I could look into them for other snippets where two String are joined soconcat-based optimization is possible.Pre-sizing of
StringBuilderforClass.gdescriptorString()andClass.getCanonicalName0()is one more improvement opportunity here.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3627/head:pull/3627$ git checkout pull/3627Update a local copy of the PR:
$ git checkout pull/3627$ git pull https://git.openjdk.java.net/jdk pull/3627/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3627View PR using the GUI difftool:
$ git pr show -t 3627Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3627.diff