[PATCH] Some improvements for java.lang.Class
Сергей Цыпанов
sergei.tsypanov at yandex.ru
Tue Mar 26 20:15:40 UTC 2019
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.levart at 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.tsypanov at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: klass.txt
Type: text/x-diff
Size: 1149 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20190326/c36dc960/klass-0001.txt>
More information about the core-libs-dev
mailing list