[PATCH] Some improvements for java.lang.Class
Peter Levart
peter.levart at gmail.com
Wed Mar 20 22:56:37 UTC 2019
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
More information about the core-libs-dev
mailing list