RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Claes Redestad]

Hi, since you've already done the work I guess I'm ok with it. :-)

While I don't expect there to be a noticeable performance difference, it
seems prudent to investigate and add a simple microbenchmark. This can
be done as a future RFE.

/Claes

On 2019-11-12 17:05, Jorn Vernee wrote:
> Hi Rémi, Claes,
> 
> Thanks for the suggestion, Rémi. I avoided using a store/load for all 
> types since I didn't want to alter the existing code shape. But, using 
> store/load for all types simplifies the generator code & javadoc, so it 
> would be nice to do that I think. However, this might have other effects 
> on peak/link-time performance as well.
> 
> Given that using store/load aligns the code-shape with what javac does, 
> I think chances are good that the JIT will have an easier time 
> optimizing that code shape. That said, it really needs more investigation.
> 
> Here is an updated webrev that uses store/load for all return types: 
> http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/
> 
> As long as Claes is okay with it, I think we should go for that one 
> since the code/doc is simpler. I can file an RFE for examining the 
> performance of different code shapes.
> 
> Thanks,
> Jorn
> 
> On 12/11/2019 16:35, Claes Redestad wrote:
>> Interesting, and you might be right. However I think it'd be better to
>> file a follow-up to examine this (pre-existing) behavior than to 
>> expand the scope of this bug fix.
>>
>> /Claes
>>
>> On 2019-11-12 16:08, Remi Forax wrote:
>>> Hi Jorn, Claes,
>>> Is it not better to always store in a local variable, like javac 
>>> does, instead of doing the double SWAP if there is a return value of 
>>> size 1 ?
>>>
>>> Rémi
>>>
>>> ----- Mail original -----
>>>> De: "Jorn Vernee" <jorn.vernee at oracle.com>
>>>> À: "Claes Redestad" <claes.redestad at oracle.com>, "core-libs-dev" 
>>>> <core-libs-dev at openjdk.java.net>
>>>> Envoyé: Mardi 12 Novembre 2019 15:56:06
>>>> Objet: Re: RFR 8233920: MethodHandles::tryFinally generates illegal 
>>>> bytecode for long/double return types
>>>
>>>> Hi Claes,
>>>>
>>>> Thanks for the comments!
>>>>
>>>> Updated webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.02/
>>>>
>>>> Also, thanks for sponsoring.
>>>>
>>>> Jorn
>>>>
>>>> On 12/11/2019 15:30, Claes Redestad wrote:
>>>>> Hi Jorn,
>>>>>
>>>>> good catch!
>>>>>
>>>>> Some minor stylistic concerns:
>>>>>
>>>>> "* = depends on whether the return type is a category 2 type, which
>>>>> takes up 2 stack slots."
>>>>>
>>>>> While long/double are known to use two stack slots, "category 2 type"
>>>>> isn't used much outside the specification. I'd either simplify to
>>>>> "* = depends on whether the return type takes up 1 or 2 stack slots."
>>>>> or add a reference to the definition of type categories.
>>>>>
>>>>> Similarly I think:
>>>>>
>>>>> boolean returnsCat2Type = returnType == long.class || returnType ==
>>>>> double.class;
>>>>>
>>>>> could be simplified as:
>>>>>
>>>>> boolean isDoubleWord = basicReturnType.isDoubleWord(); //
>>>>> returnsDoubleWord?
>>>>>
>>>>> In the test, is it possible to narrow the expected exception to the
>>>>> exact type being thrown (T1?) rather than Throwable?
>>>>>
>>>>> I can sponsor the patch.
>>>>>
>>>>> /Claes
>>>>>
>>>>> On 2019-11-12 12:09, Jorn Vernee wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the following patch that fixes handling of long/double
>>>>>> returns for the tryFinally MethodHandle combinator.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8233920
>>>>>> Webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.01/
>>>>>> (Testing=tier1)
>>>>>>
>>>>>> FWIW, I also added some missing close tags to the javadoc in the
>>>>>> InvokerBytecodeGenerator class (IntelliJ was warning about this).
>>>>>>
>>>>>> As a heads-up; I'm not a committer on the jdk project, so a sponsor
>>>>>> would be needed to push this.
>>>>>>
>>>>>> Thanks,
>>>>>> Jorn