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

[Ivan Gerasimov]

Hi Andrew!


On 4/9/19 9:05 AM, Andrew Leonard wrote:
> Hi Ivan,
> Your micro benchmarks look good, that's something I need to learn how 
> to execute.

Here's how I ran them from the command line:

make test TEST="micro:java.lang.StringBuilders.from" 
MICRO="FORK=3;WARMUP_ITER=4;ITER=6;WARMUP_TIME=4;TIME=4"

>
> Do we think this is a good resolution now?

Yes.  I'll go ahead and push it shortly.

With kind regards,
Ivan


> 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[]/
> >
> > http://cr.openjdk.java.net/~aleonard/8221430/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ealeonard/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
>
>
>
>
>
> 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

-- 
With kind regards,
Ivan Gerasimov