RFR: 8245969: Simplify String concat constant folding

[Claes Redestad]

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