----- Mail original ----- > De: "Claes Redestad" <claes.redes...@oracle.com> > À: "Remi Forax" <fo...@univ-mlv.fr> > Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net> > Envoyé: Mercredi 27 Mai 2020 17:50:19 > Objet: Re: RFR: 8245969: Simplify String concat constant folding
> 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. or one at a time :) But, as you said, this can be done later if we are able to bootstrap the indy/condy impl earlier. > >> >> 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? Ok ! > > /Claes Rémi > >> >> 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
