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

[Andrew Leonard]

Hi Ivan,
Yes, i'm happy with that, as you say the simple constructor change fixes 
the immediate issue, but not necessarily the extended issue of a 
non-compactable CharSequence in COMPACT_STRINGS mode, but that's probably 
an enhanced issue to cover in a separate bug...
I've created a new webrev.02 with just the constructor change and the 
testcase:
http://cr.openjdk.java.net/~aleonard/8221430/webrev.02/

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:     Andrew Leonard <andrew_m_leonard at uk.ibm.com>
Cc:     core-libs-dev at openjdk.java.net
Date:   26/03/2019 01:18
Subject:        Re: Request for sponsor: JDK-8221430: 
StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings 
specified



Thanks Andrew!
Introducing getCharSequenceCoder() is actually an enhancement, which may 
improve pre-allocation in certain cases.
It's not actually required to restore correctness of StringBuilder/Buffer 
constructors.
I recommend to separate it from this bug fix, so it can be discussed 
separately, and determined if this is the best approach to this 
enhancement.
If you agree, I can adjust your latest patch accordingly, run it through 
the Mach5 test systems and push it on your behalf.
With kind regards,
Ivan

On 3/25/19 5:00 PM, Andrew Leonard wrote:
Hi Ivan, 
Here is my updated webrev: 
http://cr.openjdk.java.net/~aleonard/8221430/webrev.01/ 

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:        Andrew Leonard <andrew_m_leonard at uk.ibm.com> 
Cc:        core-libs-dev at openjdk.java.net 
Date:        25/03/2019 22:55 
Subject:        Re: Request for sponsor: JDK-8221430: 
StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings 
specified 



I was thinking of organizing code similar to what is done in 
AbstractStringBuilder(int):

if (COMPACT_STRINGS && coderHint == LATIN1) {
    value = new byte[capacity];
    coder = LATIN1;
} else {
    value = StringUTF16.newBytesFor(capacity);
    coder = UTF16;
}

With kind regards,
Ivan

On 3/25/19 3:45 PM, Andrew Leonard wrote: 
Hi Ivan, 
I think I see what you're saying, you mean we also need to correct this 
line in AbstractStringBuilder 
constructor: 
value = (coder == LATIN1)
               ? new byte[capacity] : StringUTF16.newBytesFor(capacity); 
to be maybe: 
value = (COMPACT_STRINGS && coder == LATIN1)
               ? new byte[capacity] : StringUTF16.newBytesFor(capacity); 

The passed in coder stills need to be correct, since with COMPACT_STRINGS 
a string could be UTF16 if 
it cannot be compacted, so it's more than just a hint isn't it? 

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:        Andrew Leonard <andrew_m_leonard at uk.ibm.com>, 
core-libs-dev at openjdk.java.net 
Date:        25/03/2019 22:20 
Subject:        Re: Request for sponsor: JDK-8221430: 
StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings 
specified 



Hi Andrew!

Thanks for finding this bug!

Your fix solves the problem.

However, I think the main issue is that the constructor 
AbstractStringBuilder(byte,int,int) is now broken:  as you discovered, 
it allows to create a string buffer with the coder LATIN1 when 
COMPACT_STRINGS is false.

Wouldn't it make sense to rename the argument of the constructor to, 
say, coderHint, and then either use it as the coder if 
COMPACT_STRINGS==true, or discard it otherwise.

What do you think?

With kind regards,
Ivan

On 3/25/19 12:45 PM, Andrew Leonard wrote:
> Hi,
> Please can I request a sponsor for this fix to a JDK-13 regression?
>
> Patch with jtreg testcase here:
> http://cr.openjdk.java.net/~aleonard/8221430/webrev.00/
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8221430
>
> Many 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
>
> 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





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 


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


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