That’s rather elegant. Nicely done. Paul.
> On May 27, 2020, at 8:50 AM, Claes Redestad <claes.redes...@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.redes...@oracle.com> >>> À: "core-libs-dev" <core-libs-dev@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
