RFR: 8325730: StringBuilder.toString allocation for the empty String

[Aleksey Shipilev]

On Tue, 20 Feb 2024 16:32:54 GMT, Claes Redestad <redestad at openjdk.org> wrote:

> JDK-8282429 accidentally removed an optimization (JDK-8240094) that ensured StringBuilder/StringBuffer::toString returns `""` when the builders are empty.
> 
> 
> Name                         Cnt     Base      Error   Test   Error   Unit  Change
> StringBuffers.emptyToString    5   12,289 ±    0,384  9,883 ± 0,721  ns/op   1,24x (p = 0,000*)
>   :gc.alloc.rate                 1862,398 ±   57,647  0,007 ± 0,000 MB/sec   0,00x (p = 0,000*)
>   :gc.alloc.rate.norm              24,000 ±    0,000  0,000 ± 0,000   B/op   0,00x (p = 0,000*)
>   :gc.count                        31,000             0,000         counts
>   :gc.time                         21,000                               ms
> StringBuilders.emptyToString   5    4,146 ±    0,567  0,646 ± 0,003  ns/op   6,42x (p = 0,000*)
>   :gc.alloc.rate                 9208,656 ± 1234,399  0,007 ± 0,000 MB/sec   0,00x (p = 0,000*)
>   :gc.alloc.rate.norm              40,000 ±    0,000  0,000 ± 0,000   B/op   0,00x (p = 0,000*)
>   :gc.count                        96,000             0,000         counts
>   :gc.time                         64,000                               ms
>   * = significant

OK, so before [JDK-8282429](https://bugs.openjdk.org/browse/JDK-8282429) this was handled by `String{Latin1|UTF16}.newString`, gotcha.

src/java.base/share/classes/java/lang/StringBuilder.java line 478:

> 476:         }
> 477:         // Create a copy, don't share the array
> 478:         return new String(this, null);

Ok, this got me a bit confused, but I think this just inlines the call to this constructor:


    public String(StringBuilder builder) {
        this(builder, null);
    }

test/micro/org/openjdk/bench/java/lang/StringBuffers.java line 49:

> 47: 
> 48:     @Benchmark
> 49:     public String emptyToString() {

Do we actually need to remove the `substring` test here?

test/micro/org/openjdk/bench/java/lang/StringBuilderToString.java line 33:

> 31: import org.openjdk.jmh.annotations.Param;
> 32: import org.openjdk.jmh.annotations.Scope;
> 33: import org.openjdk.jmh.annotations.Setup;

Is this needed?

test/micro/org/openjdk/bench/java/lang/StringBuilders.java line 407:

> 405: 
> 406:         private void generateData() {
> 407:             sb = new StringBuilder(length);

This looks weird, given there is a `sb` initialization two lines below. Is this `sbEmpty`, really?

-------------

PR Review: https://git.openjdk.org/jdk/pull/17931#pullrequestreview-1890982087
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496181978
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496178639
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496177666
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496182725