RFR: 8246152: Improve String concat bootstrapping

[Claes Redestad]

On 2020-05-29 21:18, 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 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?

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