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

[Andrew Leonard]

Hi Ivan,
Your micro benchmarks look good, that's something I need to learn how to 
execute.
Do we think this is a good resolution now?
Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leonard at uk.ibm.com 




From:   Ivan Gerasimov <ivan.gerasimov at oracle.com>
To:     Roger Riggs <Roger.Riggs at oracle.com>, Andrew Leonard 
<andrew_m_leonard at uk.ibm.com>
Cc:     core-libs-dev at openjdk.java.net
Date:   06/04/2019 06:44
Subject:        Re: Request for sponsor: JDK-8221430: 
StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings 
specified



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[]/
>
> 
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Ealeonard_8221430_webrev.00_&d=DwID-g&c=jf_iaSHvJObTbx-siA1ZOg&r=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ&m=lU-ZYHX0l887TMjoIm22uyMKp50PG-LsAObnBAzW3ok&s=AGr15BRTPigyz4orFzK02RdhWl8vCSu18KYA0ydASrw&e=

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





Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU