On 2020-05-29 21:18, fo...@univ-mlv.fr wrote:
----- 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é: Vendredi 29 Mai 2020 20:45:26
Objet: Re: RFR: 8246152: Improve String concat bootstrapping
On 2020-05-29 19:32, fo...@univ-mlv.fr wrote:
----- 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é: Vendredi 29 Mai 2020 18:47:03
Objet: Re: RFR: 8246152: Improve String concat bootstrapping
Hi Rémi,
thanks for looking at this and suggesting some improvements!
On 2020-05-29 17:51, Remi Forax wrote:
Hi Claes,
For the code below the comment "Mock the recipe to reuse the concat generator
code",
i believe you can use String.repeat() that was introduced recently.
Sure,
The code that parse the the receipe can be in its own method to make the code
more readable,
this method returns the list and use the StringBuilder internally.
sure,
In generateMHInlineCopy,
element.get(0) and element.get(1) should be stored in local variables after
"elements.size() == 2"
will make the code more readable
sure,
In simpleConcat(),
should use a local variable 'mh', like the newString or newArrayWithSuffix
yes!
http://cr.openjdk.java.net/~redestad/8246152/open.01/
I don't think you need to check MAX_INDY_CONCAT_ARG_SLOTS anymore, given that
now indy is based on invokeWithArguments instead of invoke.
Not sure what's changed, but let's leave this outside the scope of this
RFE?
This is https://bugs.openjdk.java.net/browse/JDK-8186469
Agree that it can be changed later.
And i think you should keep the null checks of constants upfront to not change
the semantics (you are allowing constant not referenced by a recipe to be
null).
If anyone provides more constants than we use (including null values)
we'll fail the cCount != constants.length check later and throw a
StringConcatException anyway. So no real, semantic difference apart from
a change in what the exception message will say, which I think is OK,
no?
Taking a longer look at it, there seems we weren't testing for all
invariants w.r.t. mismatched number of constants. Added a few tests,
and fixed an issue where providing too few constants would have thrown
an AIOOBE instead of a SCE with my previous patch.
The catch should be:
catch (Error | RuntimeException e) {
if you want the StringConcatException to be propagated
StringConcatException is a regular Exception and is not thrown from
generateMHInlineCopy.
in that case, you should fix the comment ?
Yes, good catch!
Updated in-place.
/Claes
all the other changes looks good.
Thanks!
/Claes
Rémi
Re-running tier1
/Claes
Rémi
