RFR: 8246152: Improve String concat bootstrapping

[forax at univ-mlv.fr]

----- Mail original -----
> De: "Claes Redestad" <claes.redestad at oracle.com>
> À: "Remi Forax" <forax at univ-mlv.fr>
> Cc: "core-libs-dev" <core-libs-dev at 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, forax at univ-mlv.fr wrote:
>> ----- Mail original -----
>>> De: "Claes Redestad" <claes.redestad at oracle.com>
>>> À: "Remi Forax" <forax at univ-mlv.fr>
>>> Cc: "core-libs-dev" <core-libs-dev at 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?
> 
>> 
>> 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 ?

> 
>> 
>> all the other changes looks good.
> 
> Thanks!
> 
> /Claes


Rémi

> 
>> 
>>>
>>> Re-running tier1
>>>
>>> /Claes
>> 
>> Rémi