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+stsypa...@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
