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
