Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified

[Roger Riggs]

Hi Ivan,

The JMH number look fine.  thanks for comparing.

>     And in this most recent webrev that has separated the
>     AbstractStringBuilder
>     constructors for String from CharSequence, is it more likely that
>     the argument
>     will be an AbstractStringBuilder than a String so that comparison
>     should be done first.
>
    Yes, it's a good point!  I reordered the branches in the latest webrev.

I don't see the order changed in the webrev for AbstractStringBuilder. 
131-133.
    http://cr.openjdk.java.net/~igerasim/8221430/02/webrev/index.html

Otherwise, looks fine.

Thanks, Roger


On 04/06/2019 01:43 AM, Ivan Gerasimov wrote:
> Hi Roger!
>
> On 4/1/19 8:06 AM, Roger Riggs wrote:
>> Hi Ivan,
>>
>> Thanks for running the micro benchmarks.
>>
>> This version has more code duplication than Andrew's original
>> proposal that calculated the coder only CharSequence and had
>> a single AbstractStringBuilder constructor for computing the size
>> and allocating the byte[]/
>>
>> http://cr.openjdk.java.net/~aleonard/8221430/webrev.00/
>>
>> I'd be curious to know the JMH tests for that version compared.
>>
> That variant appeared to be slightly slower, comparing to the latest 
> variant:
>
> Here are fresh benchmarks for both variants
>
> (1) cr.openjdk.java.net/~aleonard/8221430/webrev.00 :
> Benchmark                               Mode  Cnt   Score Error Units
> StringBuilders.fromLatin1String         avgt   18  15.217 ± 0.157 ns/op
> StringBuilders.fromLatin1StringBuilder  avgt   18  19.169 ± 0.086 ns/op
> StringBuilders.fromUtf16String          avgt   18  17.593 ± 0.180 ns/op
> StringBuilders.fromUtf16StringBuilder   avgt   18  21.786 ± 0.158 ns/op
>
> (2) cr.openjdk.java.net/~igerasim/8221430/02/webrev :
> Benchmark                               Mode  Cnt   Score Error Units
> StringBuilders.fromLatin1String         avgt   18  14.655 ± 0.133 ns/op
> StringBuilders.fromLatin1StringBuilder  avgt   18  18.059 ± 0.161 ns/op
> StringBuilders.fromUtf16String          avgt   18  16.675 ± 0.124 ns/op
> StringBuilders.fromUtf16StringBuilder   avgt   18  20.761 ± 0.116 ns/op
>
> One reason might be that (2) avoids a redundant check for negative 
> length of the String argument.
>
>> Another comment is whether the 'instanceof' code is the
>> best performer for checking if the argument is a String.
>> I might think that 'seq.getClass().equals(String.class)' is faster.
>>
> Interesting.  I don't see examples of such pattern in JDK.
> Anyhow, I think that the case when a StringBuilder is constructed from 
> a String down-cast to CharSequence should be rare in practice, so it 
> is sensible to keep the code more ideomatic, i.e. use instanceof.
>
>> And in this most recent webrev that has separated the 
>> AbstractStringBuilder
>> constructors for String from CharSequence, is it more likely that the 
>> argument
>> will be an AbstractStringBuilder than a String so that comparison 
>> should be done first.
>>
> Yes, it's a good point!  I reordered the branches in the latest webrev.
>
> With kind regards,
> Ivan
>