RFR: 8246152: Improve String concat bootstrapping

[Claes Redestad]

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?

> 
> 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.

> 
> all the other changes looks good.

Thanks!

/Claes

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