I agree that getting rid of the append(concatenation) is a good move regardless; that’s just wasteful movement. The rest of the patch seems harmless.
As to applying the refactor more broadly, there’s a risk of recreating the “append(concatenation)” problem, where the concatenation is hidden inside the call to repeat(). A loop over append() is likely to be more performant than append(s.repeat()), even those the latter is more abstract and harder to get wrong. If you’re willing to take the auto-refactor result and hand-edit it to eliminate the cases where we’d create new append(concat) situations, I’d be supportive of that as well. > On Mar 26, 2019, at 4:15 PM, Сергей Цыпанов <sergei.tsypa...@yandex.ru> wrote: > > 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 Tsypanov > <klass.txt>
