On Mon, 15 Mar 2021 13:52:57 GMT, Сергей Цыпанов <github.com+10835776+stsypa...@openjdk.org> wrote:
>> A less intrusive alternative would be to use a `StringBuilder`, see changes >> in this branch: >> https://github.com/openjdk/jdk/compare/master...cl4es:stringjoin_improvement?expand=1 >> (I adapted your StringJoinerBenchmark to work with the ascii-only build >> constraint) >> >> This underperforms compared to your patch since StringBuilder.toString needs >> to do a copy, but improves over the baseline: >> Benchmark (count) >> (length) (mode) Mode Cnt Score Error Units >> StringJoinerBenchmark.stringJoiner 100 >> 64 latin avgt 5 5420.701 ± 1433.485 ns/op >> StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100 >> 64 latin avgt 5 20640.428 ± 0.130 B/op >> Patch: >> Benchmark (count) >> (length) (mode) Mode Cnt Score Error Units >> StringJoinerBenchmark.stringJoiner 100 >> 64 latin avgt 5 4271.401 ± 677.560 ns/op >> StringJoinerBenchmark.stringJoiner:·gc.alloc.rate.norm 100 >> 64 latin avgt 5 14136.294 ± 0.095 B/op >> >> The comparative benefit is that we'd avoid punching more holes into String >> implementation details for now. Not ruling that out indefinitely, but I >> think it needs a stronger motivation than to improve StringJoiner alone. > > I was thinking about `StringBuilder` at the very beginning but then decided > to have no bounds checks and reallocations. Indeed from maintainability point > of view your solution is much more attractive. I have one minor comment about > `append()`: I think we should do more chaining, e.g. > StringBuilder sb = new StringBuilder(len); > sb.append(elts[0]); > should be > StringBuilder sb = new StringBuilder(len).append(elts[0]); > etc. to have less bytecode. > > E.g. if we take > public String sb() { > StringBuilder sb = new StringBuilder(); > > sb.append("a"); > sb.append("b"); > > return sb.toString(); > } > `sb.append()` gets compiled into > L1 > LINENUMBER 23 L1 > ALOAD 1 > LDC "a" > INVOKEVIRTUAL java/lang/StringBuilder.append > (Ljava/lang/String;)Ljava/lang/StringBuilder; > POP > L2 > LINENUMBER 24 L2 > ALOAD 1 > LDC "b" > INVOKEVIRTUAL java/lang/StringBuilder.append > (Ljava/lang/String;)Ljava/lang/StringBuilder; > POP > ``` > and in case we have > ``` > sb.append("a").append("b"); > ``` > the amount of byte code is reduced: > ``` > L1 > LINENUMBER 23 L1 > ALOAD 1 > LDC "a" > INVOKEVIRTUAL java/lang/StringBuilder.append > (Ljava/lang/String;)Ljava/lang/StringBuilder; > LDC "b" > INVOKEVIRTUAL java/lang/StringBuilder.append > (Ljava/lang/String;)Ljava/lang/StringBuilder; > POP > ``` > > Also I'd change > if (addLen == 0) { > compactElts(); > return size == 0 ? "" : elts[0]; > } > to > if (size == 0) { > if (addLen == 0) { > return ""; > } > return prefix + suffix; > } > The second variant is more understandable to me and likely to be slightly > faster. > > And finally, should I close this PR and you'll create a new one from your > branch, or I need to copy your changes here? Up to you. If you adapt your PR to use a StringBuilder as suggested I can review and sponsor. ------------- PR: https://git.openjdk.java.net/jdk/pull/2627
