Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified
Roger Riggs
Roger.Riggs at oracle.com
Wed Mar 27 13:35:47 UTC 2019
Hi Ivan,
On 03/26/2019 07:30 PM, Ivan Gerasimov wrote:
> The main source of confusion here seems to be due to that the coder is
> passed in as an argument, so it either needs to be trusted or has to
> be verified in the constructor.
The design for coder is that it *is* trusted in all
internal/implementation APIs.
Subsequent changes should not weaken this invariant, it will cause doubt
in the code and cast doubt on all of the performance work that has
been done to optimize its use.
>
> So, let's instead introduce AbstractStringBuilder(CharSequence) and
> make it do all manipulations.
>
> http://cr.openjdk.java.net/~igerasim/8221430/01/webrev/
>
> Note that the common case of StringBuilder(String) already gets
> intrinsified by hotspot.
>
> What do you think?
This looks good, the logic is in one place.
Since StringBuilder(String) is an intrinsic, my doubt about a slight
performance
impact is unwarranted in that specific case.
Are there any StringBuffer/Builder jmh tests than be easily rerun to
compare?
Thanks, Roger
>
> With kind regards,
> Ivan
>
> On 3/26/19 1:04 PM, Ivan Gerasimov wrote:
>> 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
>>>
>>
>
More information about the core-libs-dev
mailing list