RFR: 8245969: Simplify String concat constant folding

[Paul Sandoz]

That’s rather elegant. Nicely done.

Paul.

> On May 27, 2020, at 8:50 AM, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> Hi Rémi,
> 
> thanks for looking at this.
> 
> On 2020-05-27 17:12, Remi Forax wrote:
>> Hi Claes,
>> so instead of having a prefix and a suffix, there is only a prefix, a suffix being seen as a prefix for the next iteration
>> and if at the end instead of just allocating an array, you allocate an array and stuff it with the last prefix.
> 
> Right, a constant will be either a prefix to an argument, or folded in
> as a suffix on the newArray combinator. Previously, only the first
> argument prepender could see a non-null prefix, so we were binding in
> a lot of null values.
> 
>> Are you sure having a suffix at the end is uncommon enough so you use @Stable instead of final for NEW_ARRAY_SUFFIX ?
> 
> The pattern to use @Stable rather than initializing to a final comes
> from a recent bootstrap stability improvement, see
> https://bugs.openjdk.java.net/browse/JDK-8218173
> 
> This doesn't matter much for bootstrap costs, and AFAICT not at all for
> the performance of the final MH.
> 
>> The name of the MH, NEW_ARRAY_SUFFIX, and the name of the method are different "newArrayPrepend", they should be identical, and i think the method should be called "newArrayWithSuffix".
> 
> Sure!
> 
>> I think i would prefer to have only one call to MethodHandles.foldArgumentsWithCombiner, with a variable combiner being either newArray() or newArrayWithSuffix(constant).
> 
> Sure!
> 
>> And
>>   constant = constant == null ? constantValue : constant + constantValue;
>> should be
>>   constant = constant == null ? constantValue : constant.concat(constantValue);
>> to avoid to create an intermediary StringBuilder (or worst if we are able to solve the boostraping issue of indy and try to use the StringConcatFactory inside itself).
> 
> Not sure I agree. Sure, .concat(..) would remove a StringBuilder, but
> that's a rather small micro-opt (back to back constants should be rather
> uncommon).
> 
> W.r.t. avoiding bootstrapping issues there are several of other string
> concatenations elsewhere already, so we wouldn't win anything unless
> we eliminate them all.
> 
>> I wonder if the code inside newArrayPrepend() can not be simplified given that we know that the suffix will be at the end of the array ?
> 
> Not by much, but we could inline part of prepend(long, byte[], String)
> to avoid a call.
> 
> http://cr.openjdk.java.net/~redestad/8245969/open.01
> 
> OK?
> 
> /Claes
> 
>> regards,
>> Rémi
>> ----- Mail original -----
>>> De: "Claes Redestad" <claes.redestad at oracle.com>
>>> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>>> Envoyé: Mercredi 27 Mai 2020 16:25:33
>>> Objet: RFR: 8245969: Simplify String concat constant folding
>>> Hi,
>>> 
>>> please review this patch to StringConcatFactory which (I think)
>>> simplifies the folding of constants around arguments by folding any
>>> suffix constant into the newArray combinator instead.
>>> 
>>> This simplifies the code in all prependers and in the general flow of
>>> the bootstrap code, at the cost of some added complexity around newArray
>>> and the new newArrayPrepend combinator. Slightly improves bootstrap and
>>> footprint overheads by not having to bind as many arguments to the
>>> prependers.
>>> 
>>> Bug:    https://bugs.openjdk.java.net/browse/JDK-8245969
>>> Webrev: http://cr.openjdk.java.net/~redestad/8245969/open.00
>>> 
>>> Testing: tier1+2
>>> 
>>> Thanks!
>>> 
>>> /Claes