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
