Hello!
As an author of the IDE inspection in question I should mention that
there's slight difference between
for(int i=0; i<n; i++) buf.append("*");
and
buf.append("*".repeat(n));
The difference appears when `n` is negative: in this case the loop
does nothing while a repeat call throws. The inspection tries its best
to preserve the semantics: unless it can prove statically that n is
never negative, the automatic replacement by default is
buf.append("*".repeat(Math.max(0, n)));
However in many cases it's still known that `n` in non-negative, so
excessive Math.max could be removed. Nevertheless I encountered cases
in our codebase when it's actually desired to ignore negative number
of repeats, so Math.max should be kept (or no replacement should be
done at all if Math.max looks too ugly). This usually possible if
iteration starts with something other than zero:
for(int i=from; i<to; i++) buf.append("*"); // do not append anything
if to < from
In this case the proposed replacement would be
buf.append("*".repeat(Math.max(0, to - from))) or
buf.append("*".repeat(to - from)) if IDE can statically prove that to
- from >= 0.
So totally mechanical replacement should work fine, but may produce
some amount of unnecessary Math.max and deciding whether to keep or
remove them requires thinking.
With best regards,
Tagir Valeev.
On Thu, Mar 21, 2019 at 1:37 AM Сергей Цыпанов
<sergei.tsypa...@yandex.ru> 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
