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

[Ivan Gerasimov]

Thanks Roger and Andrew!


On 4/9/19 10:02 AM, Roger Riggs wrote:
> 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
>
Ah, yes, I only changed it in the local copy, but didn't update the webrev.

With kind regards,
Ivan

> 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
>>
>

-- 
With kind regards,
Ivan Gerasimov