Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v3]
On Tue, 20 Feb 2024 18:42:16 GMT, Claes Redestad 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.emptyToString5 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.count31,000 0,000 counts >> :gc.time 21,000 ms >> StringBuilders.emptyToString 54,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.count96,000 0,000 counts >> :gc.time 64,000 ms >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert accidental import Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17931#issuecomment-1955020047
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v3]
On Tue, 20 Feb 2024 18:38:48 GMT, Claes Redestad 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.emptyToString5 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.count31,000 0,000 counts >> :gc.time 21,000 ms >> StringBuilders.emptyToString 54,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.count96,000 0,000 counts >> :gc.time 64,000 ms >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert accidental import Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17931#pullrequestreview-1891220541
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v3]
> 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.emptyToString5 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.count31,000 0,000 counts > :gc.time 21,000 ms > StringBuilders.emptyToString 54,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.count96,000 0,000 counts > :gc.time 64,000 ms > * = significant Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Revert accidental import - Changes: - all: https://git.openjdk.org/jdk/pull/17931/files - new: https://git.openjdk.org/jdk/pull/17931/files/40cca3e3..1846746b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17931&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17931&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17931/head:pull/17931 PR: https://git.openjdk.org/jdk/pull/17931
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v3]
On Tue, 20 Feb 2024 18:30:25 GMT, Aleksey Shipilev wrote: >> It's needed again now that I reverted the code removals.. :-) > > Mhm. I don't see any new `@Setup` methods anywhere. Just checked out the PR > locally, removed this import and benchmarks still build, and > `StringBuilderToString` bench still runs. Am I missing something here? Gah, I was looking in the wrong place. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496321177
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v2]
On Tue, 20 Feb 2024 18:04:18 GMT, Claes Redestad wrote:
>> 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);
>> }
>
> Yes, I was mostly reaching for increased consistency with `StringBuffer` here.
Good then, thanks.
>> 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?
>
> It's needed again now that I reverted the code removals.. :-)
Mhm. I don't see any new `@Setup` methods anywhere. Just checked out the PR
locally, removed this import and benchmarks still build, and
`StringBuilderToString` bench still runs. Am I missing something here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496297773
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496315384
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v2]
On Tue, 20 Feb 2024 18:08:11 GMT, Aleksey Shipilev wrote: >> I couldn't figure out why we'd want to have `String::substring` micros in a >> test `StringBuffers` (there's also a `StringSubstring` micro), though I >> could deal with that as an explicit cleanup RFE. > > Yeah, I would like to keep this change clean for backports. Do the cleanup in > a separate PR? Reverted. - PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496281834
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v2]
On Tue, 20 Feb 2024 18:00:42 GMT, Claes Redestad wrote:
>> 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?
>
> I couldn't figure out why we'd want to have `String::substring` micros in a
> test `StringBuffers` (there's also a `StringSubstring` micro), though I could
> deal with that as an explicit cleanup RFE.
Yeah, I would like to keep this change clean for backports. Do the cleanup in a
separate PR?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496277852
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v2]
On Tue, 20 Feb 2024 17:02:40 GMT, Aleksey Shipilev wrote:
>> Claes Redestad has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Revert StringBuffers removals
>> - Update from review comments by @shipilev
>
> 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);
> }
Yes, I was mostly reaching for increased consistency with `StringBuffer` 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?
It's needed again now that I reverted the code removals.. :-)
> 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?
Yes, updating the name to avoid the confusing shadowing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496273112
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496282094
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496275704
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v2]
> 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.emptyToString5 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.count31,000 0,000 counts > :gc.time 21,000 ms > StringBuilders.emptyToString 54,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.count96,000 0,000 counts > :gc.time 64,000 ms > * = significant Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Revert StringBuffers removals - Update from review comments by @shipilev - Changes: - all: https://git.openjdk.org/jdk/pull/17931/files - new: https://git.openjdk.org/jdk/pull/17931/files/7f9566b8..40cca3e3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17931&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17931&range=00-01 Stats: 41 lines in 2 files changed: 38 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17931/head:pull/17931 PR: https://git.openjdk.org/jdk/pull/17931
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String
On Tue, 20 Feb 2024 17:00:14 GMT, Aleksey Shipilev 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.emptyToString5 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.count31,000 0,000 counts
>> :gc.time 21,000 ms
>> StringBuilders.emptyToString 54,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.count96,000 0,000 counts
>> :gc.time 64,000 ms
>> * = significant
>
> 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?
I couldn't figure out why we'd want to have `String::substring` micros in a
test `StringBuffers` (there's also a `StringSubstring` micro), though I could
deal with that as an explicit cleanup RFE.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496266477
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String
On Tue, 20 Feb 2024 16:32:54 GMT, Claes Redestad 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.emptyToString5 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.count31,000 0,000 counts
> :gc.time 21,000 ms
> StringBuilders.emptyToString 54,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.count96,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
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String
On Tue, 20 Feb 2024 16:32:54 GMT, Claes Redestad 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.emptyToString5 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.count31,000 0,000 counts > :gc.time 21,000 ms > StringBuilders.emptyToString 54,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.count96,000 0,000 counts > :gc.time 64,000 ms > * = significant LGTM - Marked as reviewed by jlaskey (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17931#pullrequestreview-1890964061
RFR: 8325730: StringBuilder.toString allocation for the empty String
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.emptyToString5 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.count31,000 0,000 counts :gc.time 21,000 ms StringBuilders.emptyToString 54,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.count96,000 0,000 counts :gc.time 64,000 ms * = significant - Commit messages: - Copyrights - 8325730: StringBuilder.toString allocation for the empty String Changes: https://git.openjdk.org/jdk/pull/17931/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17931&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325730 Stats: 53 lines in 5 files changed: 14 ins; 31 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17931/head:pull/17931 PR: https://git.openjdk.org/jdk/pull/17931
