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

Roger Riggs Roger.Riggs at oracle.com
Tue Mar 26 19:45:33 UTC 2019 

Hi Leonard,

The introduction (in -01) of getCharSequenceCoder() and its use in 
StringBuilder and StringBuffer are fine.

The change that is unnecessary is the change to the constructor in 
AbstractStringBuilder(coder, length, addition) to check COMPACT_STRINGS
before using the coder.
The existing code that uses the coder literally is fine since it is
computed from getCharSequenceCoder().

I should have been more specific.

Roger


On 03/26/2019 03:37 PM, Andrew Leonard wrote:
> So my creation of getCharSequenceCoder() was in trying to use the same 
> method as the StringBuffer(String) constructors, ie.String.coder(), 
> however with the StringBuffer(CharSequence,..) constructor the seq can 
> be a String or a StringBuilder or a StringBuffer, hence I created the 
> static getCharSequenceCoder() which switched on the sequence type to 
> obtain the coder...
> Isn't that in keeping with the (String) constructor? i'm not sure how 
> else we can determine the coder from a CharSequence ? unless we change 
> the constructor to delegate to the caller to pass in the coder?
>
> 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>
> Cc: core-libs-dev at openjdk.java.net, Ivan Gerasimov 
> <ivan.gerasimov at oracle.com>
> Date: 26/03/2019 19:15
> Subject: Re: Request for sponsor: JDK-8221430: 
> StringBuffer(CharSequence) constructor truncates when 
> -XX:-CompactStrings specified
> ------------------------------------------------------------------------
>
>
>
> 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_ 
> <mailto:andrew_m_leonard at uk.ibm.com>
>
>
>
>
> From: Roger Riggs _<Roger.Riggs at oracle.com>_ 
> <mailto:Roger.Riggs at oracle.com>
> To: Andrew Leonard _<andrew_m_leonard at uk.ibm.com>_ 
> <mailto:andrew_m_leonard at uk.ibm.com>, Ivan Gerasimov 
> _<ivan.gerasimov at oracle.com>_ <mailto:ivan.gerasimov at oracle.com>
> Cc: _core-libs-dev at openjdk.java.net_ 
> <mailto: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
>
>
>
> 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