It' seems messages sent from Yandex Mail mobile application are not delivered, so I repeat it from web app.
--------------------------- Hello, Yes I’m willing to take this As I understand there is the code imported from 3rd party projects which shouldn’t be touched. I know only about jdk.internal.org.objectweb.* Are there any other similar places? Also what about the task to bind the changes to? I’m talking about this one and changes to java.lang.Class::methodToString. Sergei Tsypanov 22:03, 3 апреля 2019 г., Brian Goetz <brian.go...@oracle.com>: > 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> -- Отправлено из мобильного приложения Яндекс.Почты
