Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified
Ivan Gerasimov
ivan.gerasimov at oracle.com
Tue Mar 26 17:21:54 UTC 2019
On 3/26/19 8: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..
>
Right. I agree it would be nice to add this as an enhancement, though I
believe it would be clearer to fix one issue at a time.
Personally, I like what you have in webrev-02, as it fixes the issue
with wrong coder when compact strings are disabled.
(The only minor change left is to add the bug id to the list of bugs in
the regression test.)
For the enhancement with determining the coder of the argument, I was
thinking if it may be sensible to extend it to general CharSequence also
(i.e. scan it in advance and see if all characters can be encoded with
LATIN1).
Maybe not.
That's why I think it should be discussed within different request.
With kind regards,
Ivan
> Which patch do we favour? webrev-01 or -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: Roger Riggs <Roger.Riggs at oracle.com>
> To: 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>
> ------------------------------------------------------------------------
>
>
>
> 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
> >
> >
> >
> >
> > 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/
> <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
> >
> >
> >
> >
> > 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/
> <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
> >>
> >> 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