Hello Peter,
I was unaware of mentioned detail (thank you a lot for pointing that out, it
made me read the whole JEP280 again) so I've rebenchmarked my changes compiled
with -XDstringConcat=inline.
1) as of Class::getTypeName for Object[] it turned out, that String::repeat
performs slightly better:
Benchmark Mode Cnt
Score Error Units
getTypeName_patched avgt 25
36,676 ± 3,559 ns/op
getTypeName avgt 25
39,083 ± 1,737 ns/op
getTypeName_patched:·gc.alloc.rate.norm avgt 25
152,000 ± 0,001 B/op
getTypeName:·gc.alloc.rate.norm avgt 25
152,000 ± 0,001 B/op
But for Object[][] situation is the opposite:
Mode Cnt Score
Error Units
getTypeName_patched avgt 50 47,045
± 0,455 ns/op
getTypeName avgt 50 40,536
± 0,196 ns/op
getTypeName_patched:·gc.alloc.rate.norm avgt 50 176,000
± 0,001 B/op
getTypeName:·gc.alloc.rate.norm avgt 50 152,000
± 0,001 B/op
2) as of Class::methodToString patched version clearly wins:
methodToString_1arg avgt 50 238,476
± 1,641 ns/op
methodToString_1arg_patched avgt 50 197,900
± 5,824 ns/op
methodToString_1arg:·gc.alloc.rate.norm avgt 50 1209,600
± 6,401 B/op
methodToString_1arg_patched:·gc.alloc.rate.norm avgt 50 936,000
± 0,001 B/op
methodToString_noArgs avgt 50 224,054
± 1,840 ns/op
methodToString_noArgs_patched avgt 50 78,195
± 0,418 ns/op
methodToString_noArgs:·gc.alloc.rate.norm avgt 50 1131,200
± 7,839 B/op
methodToString_noArgs_patched:·gc.alloc.rate.norm avgt 50 408,000
± 0,001 B/op
As Brian suggested I've separated the changes. The patch attached contains only
the changes for Class::methodToString. They are obviously helpful as they rid
concatenation as argument of StringBuilder::append call (obvious antipattern to
me) and skip Stream creation for empty array.
String::repeat obviously requires more investigation, it might turn out we
don't need it in java.base in majority of use cases or in all of them.
Regards,
Sergei Tsypanov
21.03.2019, 00:56, "Peter Levart" <peter.lev...@gmail.com>:
> Hi Sergei,
>
> I don't know if you are aware that the new invokedynamic based
> translation of string concatenation expressions introduced in JDK 9
> (http://openjdk.java.net/jeps/280) only applies to code outside
> java.base module. java.base module is still compiled with old-style
> StringBuilder based translation of string concatenation expressions
> because of bootstraping issues.
>
> If your benchmarks bellow are compiled with option
> -XDstringConcat=inline to disable JEP280 translation (as java.base
> module is), then it's OK. Else you are not benchmarking the right
> translation of the code as you intend to change the code in java.base
> module.
>
> Regards, Peter
>
> On 3/20/19 7:35 PM, Сергей Цыпанов wrote:
>> I had a brief conversation with Brian Goetz which has left off the mailing
>> list for some reason. Here's the text:
>> ---------------------------
>> Brian:
>>
>> These enhancements seem reasonable; these are exactly the cases that
>> String::repeat was intended for. (This is a “is this a reasonable idea”
>> review, not a code review.).
>> Have you looked for other places where similar transformations could be
>> done?
>>
>> ---------------------------
>> Me:
>>
>> Hello,
>> after I had realized that looped StringBuilder::append calls can sometimes
>> be replaced with String::repeat I requested corresponding inspection in IDEA
>> issue tracker.
>> Using the inspection I’ve found 129 similar warnings in jdk13 repo.
>> Some of them are very promising, e.g. BigDecimal:: toPlainString where
>> currently we have
>>
>>> int trailingZeros = checkScaleNonZero((-(long)scale));
>>> StringBuilder buf;
>>> if(intCompact!=INFLATED) {
>>> buf = new StringBuilder(20+trailingZeros);
>>> buf.append(intCompact);
>>> } else {
>>> String str = intVal.toString();
>>> buf = new StringBuilder(str.length()+trailingZeros);
>>> buf.append(str);
>>> }
>>> for (int i = 0; i < trailingZeros; i++) {
>>> buf.append('0');
>>> }
>>> return buf.toString();
>> which in fact can be simplified to
>>
>>> int trailingZeros = checkScaleNonZero((-(long)scale));
>>> if(intCompact!=INFLATED) {
>>> return intCompact + "0".repeat(trailingZeros);
>>> } else {
>>> return intVal.toString() + "0".repeat(trailingZeros);
>>> }
>> The second solution is not only shorter and more simple but it likely to be
>> significantly faster and memory-saving.
>> BTW, your reply to original message for some reason is missing from
>> web-view.
>>
>> ---------------------------
>> Brian:
>>
>> Cool. Note that replacing append() calls with repeat is not necessarily a
>> win for anything other than code compactness; String::repeat will create a
>> new string, which will then get appended to another StrinBuilder. So when
>> used correctly (such as presized StringBuilders) appending in a loop is
>> probably just as fast (look at the implementation of String::repeat.).
>>
>>> if(intCompact!=INFLATED) {
>>> return intCompact + "0".repeat(trailingZeros);
>>> } else {
>>> return intVal.toString() + "0".repeat(trailingZeros);
>>> }
>> Which can be further simplified to
>>
>>> ((intCompact != INFLATED) ? intCompact : intVal.toString) +
>>> “0”.repeat(trailingZeros);
>>
>> The second solution is not only shorter and more simple but it likely to be
>> significantly faster and memory-saving.
>> I like short and simple. I am questioning the “faster and memory saving.”
>> That should be validated.
>>
>> ---------------------------
>> Me:
>>
>> I think optimizations provided by JEP 280 allow compiler to optimize String
>> concatenation executed with '+' by using StringBuilder of exact size (when
>> the size of all components is known, like in our case).
>>
>> I've checked this with benchmark
>>
>>> @State(Scope.Thread)
>>> @BenchmarkMode(Mode.AverageTime)
>>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>>> public class ConcatBenchmark {
>>>
>>> @Param({"1", "2", "5", "8"})
>>> private int trailingZeros;
>>>
>>> private final long intCompact = 1L;
>>>
>>> @Benchmark
>>> public String stringBuilder() {
>>> StringBuilder buf;
>>> buf = new StringBuilder(20 + trailingZeros);
>>> buf.append(intCompact);
>>> for (int i = 0; i < trailingZeros; i++) {
>>> buf.append('0');
>>> }
>>> return buf.toString();
>>> }
>>> @Benchmark
>>> public String stringRepeat() {
>>> return intCompact + "0".repeat(trailingZeros);
>>> }
>>> }
>> Results:
>> trailingZeros Mode Cnt Score Error
>> Units
>> stringBuilder 1 avgt 15 17,683 ± 0,664 ns/op
>> stringRepeat 1 avgt 15 9,052 ± 0,042 ns/op
>>
>> stringBuilder 2 avgt 15 19,053 ± 1,206 ns/op
>> stringRepeat 2 avgt 15 15,864 ± 1,331 ns/op
>>
>> stringBuilder 5 avgt 15 25,839 ± 2,256 ns/op
>> stringRepeat 5 avgt 15 19,290 ± 1,685 ns/op
>>
>> stringBuilder 8 avgt 15 26,488 ± 0,371 ns/op
>> stringRepeat 8 avgt 15 19,420 ± 1,593 ns/op
>>
>> stringBuilder:·gc.alloc.rate.norm 1 avgt 15 88,000 ± 0,001 B/op
>> stringRepeat:·gc.alloc.rate.norm 1 avgt 15 48,000 ± 0,001 B/op
>>
>> stringBuilder:·gc.alloc.rate.norm 2 avgt 15 88,000 ± 0,001 B/op
>> stringRepeat:·gc.alloc.rate.norm 2 avgt 15 72,000 ± 0,001 B/op
>>
>> stringBuilder:·gc.alloc.rate.norm 5 avgt 15 96,000 ± 0,001 B/op
>> stringRepeat:·gc.alloc.rate.norm 5 avgt 15 72,000 ± 0,001 B/op
>>
>> stringBuilder:·gc.alloc.rate.norm 8 avgt 15 104,000 ± 0,001 B/op
>> stringRepeat:·gc.alloc.rate.norm 8 avgt 15 80,000 ± 0,001 B/op
>>
>> I think this is a useful optimization, so we should use String::repeat
>> where possible.
>>
>> ---------------------------
>> Brian:
>>
>> My recommendation is to split your patch into two. The first is the
>> straightforward “replace loop with repeat” refactoring, which can be
>> mechanically generated by the IDE. Here, you should examine each site to
>> ensure that the transform seems sensible given the context. The second can
>> then be additional hand-refactoring opportunities exposed by the first. The
>> benefit of splitting it this way is that reviewing the first is far easier
>> when you can say all the changes are mechanical.
>>
>> (Somehow this fell off the list; feel free to bring it back.)
>>
>> 18.03.2019, 21:13, "Сергей Цыпанов" <sergei.tsypa...@yandex.ru>:
>>> Hi,
>>>
>>> I have an enhancement proposal for java.lang.Class.methodToString and
>>> java.lang.Class.getTypeName.
>>>
>>> First one is used when NoSuchMethodException is thrown from
>>> Class::getMethod which is in turn widely used in Spring Framework and often
>>> throws.
>>>
>>> In current implementation we have 2 major problems:
>>>
>>> - we create stream for the case when argTypes is not null but empty (which
>>> happens e. g. when Class::getMethod is called without vararg and throws)
>>> - we have torn StringBuilder::append chain
>>>
>>> I’ve modified the method to skip creation of Stream for empty array and
>>> used separate StringBuilder for each case. Latter allowed to rid SB
>>> completely and use invokedynamic-based concatenation.
>>>
>>> I’ve compared both approaches for 2 cases:
>>>
>>> 1) argTypes is empty
>>> 2) argTypes.length == 1
>>>
>>> Benchmark Mode Cnt Score Error Units
>>>
>>> methodToString_noArgs avgt 25 170,986 ± 5,708 ns/op
>>> methodToString_noArgs_patched avgt 25 26,883 ± 2,906 ns/op
>>>
>>> methodToString_1arg avgt 25 183,012 ± 0,701 ns/op
>>> methodToString_1arg_patched avgt 25 112,784 ± 0,920 ns/op
>>>
>>> methodToString_noArgs:·gc.alloc.rate.norm avgt 25 881,600 ± 9,786 B/op
>>> methodToString_noArgs_patched:·gc.alloc.rate.norm avgt 25 128,000 ± 0,001
>>> B/op
>>>
>>> methodToString_1arg:·gc.alloc.rate.norm avgt 25 960,000 ± 0,001 B/op
>>> methodToString_1arg_patched:·gc.alloc.rate.norm avgt 25 552,000 ± 0,001
>>> B/op
>>>
>>> We have the same problem regarding misusage of StringBuilder in Class::
>>> getTypeName:
>>>
>>> StringBuilder sb = new StringBuilder();
>>> sb.append(cl.getName());
>>> for (int i = 0; i < dimensions; i++) {
>>> sb.append("[]");
>>> }
>>> return sb.toString();
>>>
>>> I suggest to use String::repeat instead of the loop: this again allows to
>>> get rid of StringBuilder and replace mentioned code with
>>>
>>> return cl.getName() + "[]".repeat(dimensions);
>>>
>>> Here are benchmark results executed for type Object[].class:
>>>
>>> Mode Cnt Score Error Units
>>> getTypeName_patched avgt 25 16,037 ± 0,431 ns/op
>>> getTypeName_patched:·gc.alloc.rate.norm avgt 25 64,000 ± 0,001 B/op
>>>
>>> getTypeName avgt 25 34,274 ± 1,432 ns/op
>>> getTypeName:·gc.alloc.rate.norm avgt 25 152,000 ± 0,001 B/op
>>>
>>> Regards,
>>> Sergei Tsypanovdiff --git a/src/java.base/share/classes/java/lang/Class.java b/src/java.base/share/classes/java/lang/Class.java
--- a/src/java.base/share/classes/java/lang/Class.java
+++ b/src/java.base/share/classes/java/lang/Class.java
@@ -3417,14 +3417,13 @@
* Helper method to get the method name from arguments.
*/
private String methodToString(String name, Class<?>[] argTypes) {
- StringBuilder sb = new StringBuilder();
- sb.append(getName() + "." + name + "(");
- if (argTypes != null) {
- sb.append(Stream.of(argTypes).map(c -> {return (c == null) ? "null" : c.getName();}).
- collect(Collectors.joining(",")));
+ if (argTypes == null || argTypes.length == 0) {
+ return getName() + '.' + name + "()";
}
- sb.append(")");
- return sb.toString();
+ return getName() + '.' + name
+ + Arrays.stream(argTypes)
+ .map(c -> c == null ? "null" : c.getName())
+ .collect(Collectors.joining(",", "(", ")"));
}
/** use serialVersionUID from JDK 1.1 for interoperability */
