Okay, that makes sense. Thanks for clarifying. Greetings, Michael
Am 13. Mai 2021 00:45:13 MESZ schrieb Claes Redestad <claes.redes...@oracle.com>: >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
