Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified
Ivan Gerasimov
ivan.gerasimov at oracle.com
Tue Mar 26 20:04:49 UTC 2019
From the design point of view, I believe it is better to have the
constructor AbstractStringBuilder(int, int, int) to check if the coder
argument makes sense with respect to the value of COMPACT_STRING, so it
won't be possible to create a StringBuilder with the coder==LATIN1, when
it is not supported.
For calculating the coderHint then, it is not necessary to check
COMPACT_STRING: If the CharSequence argument is in fact String or
AbstractStringBuilder, the coder is known, otherwise LATIN1 can be
passed in as a hint (for an arbitrary CharSequence it is not 100%
accurate anyway).
The constructor AbstractStringBuilder(int, int, int) will then either
use the provided coder, or adjust it if necessary.
Will we agree on something like following?
http://cr.openjdk.java.net/~igerasim/8221430/00/webrev/
With kind regards,
Ivan
On 3/26/19 12:14 PM, Roger Riggs wrote:
> Hi,
>
> We've got the subject open and its fresh, there's no need for a
> separate review cycle.
>
> The first fix (-01) does not seem to be consistent with the original
> design
> and handling of the coder. The StringBuilder(String) and
> StringBuffer(String)
> constructors are the pattern that should be followed for determining
> the coder for the new instance.
>
> Checking for COMPACT_STRING in two places (the AbstractStringBuilder and
> the sub classes) is unnecessary and distributes the information about the
> correct coder across two classes where determining what it should be
> in the subclass has more complete information and can correctly
> determine
> the coder once.
>
> We can likely find a reviewer to be a tie-breaker if Ivan sees it as
> desirable.
>
> Thanks, Roger
>
>
> On 03/26/2019 02:38 PM, Andrew Leonard wrote:
>> Sorry chaps, I think my brain is getting confused!, I think we have
>> conflicting reviews here?
>>
>> Roger, I added the getCharSequenceCoder() to AbstractStringBuilder so
>> it was only defined in one place..
>> I agree with this being called in StringBuffer/Builder then we don't
>> need the change to AbstractStringBuild() constuctor, however Ivan
>> wants getCharSequenceCoder() that done as a separate "bug".
>>
>> So I think it comes down to do we do this as 2 "bugs" or 1 ?
>>
>> 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: Roger Riggs <Roger.Riggs at oracle.com>
>> To: Andrew Leonard <andrew_m_leonard at uk.ibm.com>, Ivan Gerasimov
>> <ivan.gerasimov at oracle.com>
>> Cc: core-libs-dev at openjdk.java.net
>> Date: 26/03/2019 18:19
>> Subject: Re: Request for sponsor: JDK-8221430:
>> StringBuffer(CharSequence) constructor truncates when
>> -XX:-CompactStrings specified
>> ------------------------------------------------------------------------
>>
>>
>>
>> Hi Andrew,
>>
>> You are going to have to change the same code twice
>> because the changes should be the StringBuffer and StringBuilder
>> constructors and would remove the code that is added to
>> the AbstractStringBuilder constructor. That's a waste of review cycles.
>>
>>
>> On 03/26/2019 11:45 AM, Andrew Leonard wrote:
>> Hi Roger,
>> No worries, the more the merrier!
>> So that was one of my reasoning for adding getCharSequenceCode() was,
>> I think what you're suggesting is my webrev.01,
>> _http://cr.openjdk.java.net/~aleonard/8221430/webrev.01/_
>> <http://cr.openjdk.java.net/%7Ealeonard/8221430/webrev.01/>
>> Ivan's view is that behaviour was an extended issue, which it is, but
>> I thought it was nice to add..
>>
>> Which patch do we favour? webrev-01 or -02 ?
>>
>> Neither, there should be no change to the AbstractStringBuilder
>> constructor
>> and the change should be done in the subclass constructors.
>>
>> Roger
>>
>>
>> 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_
>> <mailto:andrew_m_leonard at uk.ibm.com>
>>
>>
>>
>>
>> From: Roger Riggs _<Roger.Riggs at oracle.com>_
>> <mailto:Roger.Riggs at oracle.com>
>> To: _core-libs-dev at openjdk.java.net_
>> <mailto:core-libs-dev at openjdk.java.net>
>> Date: 26/03/2019 15:24
>> Subject: Re: Request for sponsor: JDK-8221430:
>> StringBuffer(CharSequence) constructor truncates when
>> -XX:-CompactStrings specified
>> Sent by: "core-libs-dev" _<core-libs-dev-bounces at openjdk.java.net>_
>> <mailto:core-libs-dev-bounces at openjdk.java.net>
>> ------------------------------------------------------------------------
>>
>>
>>
>> Hi Andrew,
>>
>> Sorry to be late to the review.
>>
>> For symmetry with the constructors StringBuffer(String), and
>> StringBuilder(String)
>> the determine the coder based on the input argument, I would recommend
>> using the getCharSequenceCoder added in the -01 webrev and calling
>> it from the StringBuffer(CharSeq...), and StringBuilder(CharSeq...)
>> constructors.
>>
>> It would be symmetric with the getCoder() method (line 1635)
>> and select the appropriate coder base on the input value (if known.)
>>
>> Thanks, Roger
>>
>>
>>
>> On 03/26/2019 10:57 AM, Andrew Leonard wrote:
>> > 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/_
>> <http://cr.openjdk.java.net/%7Ealeonard/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_
>> <mailto:andrew_m_leonard at uk.ibm.com>
>> >
>> >
>> >
>> >
>> > From: Ivan Gerasimov _<ivan.gerasimov at oracle.com>_
>> <mailto:ivan.gerasimov at oracle.com>
>> > To: Andrew Leonard _<andrew_m_leonard at uk.ibm.com>_
>> <mailto:andrew_m_leonard at uk.ibm.com>
>> > Cc: _core-libs-dev at openjdk.java.net_
>> <mailto: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/_
>> <http://cr.openjdk.java.net/%7Ealeonard/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_
>> <mailto:andrew_m_leonard at uk.ibm.com>
>> >
>> >
>> >
>> >
>> > From: Ivan Gerasimov _<ivan.gerasimov at oracle.com>_
>> <mailto:ivan.gerasimov at oracle.com>
>> > To: Andrew Leonard _<andrew_m_leonard at uk.ibm.com>_
>> <mailto:andrew_m_leonard at uk.ibm.com>
>> > Cc: _core-libs-dev at openjdk.java.net_
>> <mailto: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_
>> <mailto:andrew_m_leonard at uk.ibm.com>
>> >
>> >
>> >
>> >
>> > From: Ivan Gerasimov _<ivan.gerasimov at oracle.com>_
>> <mailto:ivan.gerasimov at oracle.com>
>> > To: Andrew Leonard _<andrew_m_leonard at uk.ibm.com>_
>> <mailto:andrew_m_leonard at uk.ibm.com>,
>> > _core-libs-dev at openjdk.java.net_
>> <mailto: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/_
>> <http://cr.openjdk.java.net/%7Ealeonard/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_
>> <mailto: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
>>
>>
>>
>>
>>
>> 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
>>
>>
>>
>> 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
More information about the core-libs-dev
mailing list