RFR: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available
Claes Redestad
claes.redestad at oracle.com
Wed May 12 22:45:13 UTC 2021
Hi,
I gave this some thought the other day when looking at one of Sergey's
patches.
I'm no expert on javac but I think translating to String::concat is
likely to be a bit harder than it seems. It's not a static method, so
the left hand side must be non-null. This is something I believe javac
can't prove in the general case. Corner cases such as when the left
hand side is a String literal, of course, but that would be of limited
utility.
There are more convoluted solutions that might work if one squints (add
public static concat methods and translate calls into those?), but the
question we need to be asking ourselves is really if it is worth our
time - and the added complexity? The answer is likely no.
It might help some if you're targeting java 8 or using javac as a
frontend for non-JVM targets (lol!), but going forward I think it would
mostly help java.base and a few other JDK modules where the
invokedynamic translation can't be used due bootstrapping issues. And in
most of those cases it won't really matter for performance anyhow.
That's why I think using String::concat on a case-by-case basis in those
few OpenJDK modules where indified string concat is not applicable is
enough. When we see that it helps. And then only sparingly.
Hope this clarifies how I reason about this.
/Claes
On 2021-05-12 22:49, Michael Kroll wrote:
> Hello,
>
> just being curious here:
> Before we start to change every usage of String+String into String.concat, shouldn't the compiler be so smart to do that for us?
> Currently it compiles to invokedynamic if available and to using StringBuilder otherwise. Now why doesn't it compile to String.concat instead of StringBuilder for the case when invokedynamic is not available as target?
>
> greets,
> Michael
>
>
>
>
> Am 12. Mai 2021 15:04:21 MESZ schrieb "Сергей Цыпанов" <github.com+10835776+stsypanov at openjdk.java.net>:
>> Hello, from discussion in https://github.com/openjdk/jdk/pull/3464 I've
>> found out, that in a few of JDK core classes, e.g. in `j.l.Class`
>> expressions like `baseName.replace('.', '/') + '/' + name` are not
>> compiled into `invokedynamic`-based code, but into one using
>> `StringBuilder`. This happens due to some bootstraping issues.
>>
>> However the code like
>>
>> private String getSimpleName0() {
>> if (isArray()) {
>> return getComponentType().getSimpleName() + "[]";
>> }
>> //...
>> }
>>
>> can be improved via replacement of `+` with `String.concat()` call.
>>
>> I've used this benchmark to measure impact:
>>
>> @State(Scope.Thread)
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class ClassMethodsBenchmark {
>> private final Class<? extends Object[]> arrayClass = Object[].class;
>> private Method descriptorString;
>>
>> @Setup
>> public void setUp() throws NoSuchMethodException {
>> //fore some reason compiler doesn't allow me to call
>> Class.descriptorString() directly
>> descriptorString = Class.class.getDeclaredMethod("descriptorString");
>> }
>>
>> @Benchmark
>> public Object descriptorString() throws Exception {
>> return descriptorString.invoke(arrayClass);
>> }
>>
>> @Benchmark
>> public Object typeName() {
>> return arrayClass.getTypeName();
>> }
>> }
>>
>> and got those results
>>
>> Mode Cnt Score Error Units
>> descriptorString avgt 60 91.480
>> ± 1.839 ns/op
>> descriptorString:·gc.alloc.rate.norm avgt 60 404.008
>> ± 4.033 B/op
>> descriptorString:·gc.churn.G1_Eden_Space avgt 60 2791.866
>> ± 181.589 MB/sec
>> descriptorString:·gc.churn.G1_Eden_Space.norm avgt 60 401.702
>> ± 26.047 B/op
>> descriptorString:·gc.churn.G1_Survivor_Space avgt 60 0.003
>> ± 0.001 MB/sec
>> descriptorString:·gc.churn.G1_Survivor_Space.norm avgt 60 ≈ 10⁻³
>> B/op
>> descriptorString:·gc.count avgt 60 205.000
>> counts
>> descriptorString:·gc.time avgt 60 216.000
>> ms
>>
>> patched
>> Mode Cnt Score Error Units
>> descriptorString avgt 60 65.016
>> ± 3.446 ns/op
>> descriptorString:·gc.alloc.rate avgt 60 2844.240
>> ± 115.719 MB/sec
>> descriptorString:·gc.alloc.rate.norm avgt 60 288.006
>> ± 0.001 B/op
>> descriptorString:·gc.churn.G1_Eden_Space avgt 60 2832.996
>> ± 206.939 MB/sec
>> descriptorString:·gc.churn.G1_Eden_Space.norm avgt 60 286.955
>> ± 17.853 B/op
>> descriptorString:·gc.churn.G1_Survivor_Space avgt 60 0.003
>> ± 0.001 MB/sec
>> descriptorString:·gc.churn.G1_Survivor_Space.norm avgt 60 ≈ 10⁻³
>> B/op
>> descriptorString:·gc.count avgt 60 208.000
>> counts
>> descriptorString:·gc.time avgt 60 228.000
>> ms
>> -----------------
>> typeName avgt 60 34.273
>> ± 0.480 ns/op
>> typeName:·gc.alloc.rate avgt 60 3265.356
>> ± 45.113 MB/sec
>> typeName:·gc.alloc.rate.norm avgt 60 176.004
>> ± 0.001 B/op
>> typeName:·gc.churn.G1_Eden_Space avgt 60 3268.454
>> ± 134.458 MB/sec
>> typeName:·gc.churn.G1_Eden_Space.norm avgt 60 176.109
>> ± 6.595 B/op
>> typeName:·gc.churn.G1_Survivor_Space avgt 60 0.003
>> ± 0.001 MB/sec
>> typeName:·gc.churn.G1_Survivor_Space.norm avgt 60 ≈ 10⁻⁴
>> B/op
>> typeName:·gc.count avgt 60 240.000
>> counts
>> typeName:·gc.time avgt 60 255.000
>> ms
>>
>> patched
>>
>> typeName avgt 60 15.787
>> ± 0.214 ns/op
>> typeName:·gc.alloc.rate avgt 60 2577.554
>> ± 32.339 MB/sec
>> typeName:·gc.alloc.rate.norm avgt 60 64.001
>> ± 0.001 B/op
>> typeName:·gc.churn.G1_Eden_Space avgt 60 2574.039
>> ± 147.774 MB/sec
>> typeName:·gc.churn.G1_Eden_Space.norm avgt 60 63.895
>> ± 3.511 B/op
>> typeName:·gc.churn.G1_Survivor_Space avgt 60 0.003
>> ± 0.001 MB/sec
>> typeName:·gc.churn.G1_Survivor_Space.norm avgt 60 ≈ 10⁻⁴
>> B/op
>> typeName:·gc.count avgt 60 189.000
>> counts
>> typeName:·gc.time avgt 60 199.000
>> ms
>>
>> 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 to `invokedynamic`) so
>> I could look into them for other snippets where two String are joined
>> so `concat`-based optimization is possible.
>>
>> Pre-sizing of `StringBuilder` for `Class.gdescriptorString()` and
>> `Class.getCanonicalName0()` is one more improvement opportunity here.
>>
>> -------------
>>
>> Commit messages:
>> - Use String.concat() where invokedynamic-based String concatenation is
>> not available
>>
>> Changes: https://git.openjdk.java.net/jdk/pull/3627/files
>> Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3627&range=00
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8266972
>> Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
>> Patch: https://git.openjdk.java.net/jdk/pull/3627.diff
>> Fetch: git fetch https://git.openjdk.java.net/jdk
>> pull/3627/head:pull/3627
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3627
More information about the core-libs-dev
mailing list