[foreign] RFR 8215289: Cleanup code post SystemABI integration

[Sundararajan Athijegannathan]

Looks good.

-Sundar

On 13/12/18, 5:17 PM, Maurizio Cimadamore wrote:
> Quick followup which addresses comments from Sundar:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8215289_v2/
>
> (only changes are in NativeMethodType).
>
> @Jorn: yes, CallingConvention is probably redundant, and it occurred 
> to me that we can achieve same effect by letting a platform expose 
> multiple ABIs instances. E.g. you can do SystemABI.getInstance(...) 
> and get an ABI for the calling convention you want. This is, from a 
> modeling perspective, not 100% accurate (in abstract terms, calling 
> conventions sit under the ABI umbrella), but I think it's a good 
> pragmatic API choice.
>
> I will followup with more simplifications; I think I'll also put 
> j.l.r.Cleaner back on the table for upcall symbols, since now 
> allocation of callbacks occurs via a Scope, so, the Scope guarantees 
> there's always and hard reference to it (until the scope is closed, of 
> course). Which would also allow us to reclaim the 
> SystemABI::freeUpcallStub method.
>
> Maurizio
>
> On 13/12/2018 04:25, Sundararajan Athijegannathan wrote:
>> * NativeMethodType class could be final (given that constructor is 
>> private it is effectively final anyway - and this API looks more or 
>> less like MethodType for panama)
>>
>> * methodType and function fields of NativeMethodType could be volatile.
>>
>> * @throws IndexOutOfBoundsException is missing for 
>> NativeMethodType.parameterType().
>>
>> * We need checks for parameter types being non-null (either in the 
>> constructor or in the ".of" static method).
>>
>> Otherwise, looks good.
>>
>> PS. I built & tested the patch on Mac. All tests fine on Mac.
>>
>> Thanks,
>> -Sundar
>>
>>
>> On 12/12/18, 9:48 PM, Maurizio Cimadamore wrote:
>>> Hi,
>>> this is a followup to the SystemABI work; the goal is to clean up 
>>> some of the code, as I found some minor issues.
>>>
>>> The most important issue is that there is a bug in the 
>>> shortcircuiting logic - e.g. when doing a downcall on an upcall 
>>> stub. In these cases, we return the method handle cached in the 
>>> upcall handler, but this method handle doesn't always have the same 
>>> signature as the one that has been requested by the client. The most 
>>> striking case is UniversalUpcallHandler, whose method handle is 
>>> always of type (Object[]).
>>>
>>> Since we also uncovered another issue with this logic during the 
>>> review process (there are questions as to whether the MH can be 
>>> safely recycled if the new native method type is different from the 
>>> old one), I decided to simplify the code and remove the logic 
>>> altogether. If performances indicate that this is an issue, we can 
>>> work out a story; I did some JMH benchamarks and found no evidence 
>>> that this is the case right now (but we can revise if needed).
>>>
>>> I also did a bunch of other changes:
>>>
>>> * consolidated NativeMethodType; the class is now just an aggregate 
>>> of layout types; the constructor that takes a Method and a Function 
>>> has been moved into Utils, after all this is a binder-specific 
>>> functionality (as the binder happens to know how to go from an 
>>> interface/function to a NMT). I also added accessors in the spirit 
>>> of j.l.i.MethodType to get return and parameter types.
>>>
>>> * I also added javadoc to all public members of NMT
>>>
>>> * I changed all invokers to use NMT where possible, instead of 
>>> arrays of LayoutType
>>>
>>> * I reverted changes in order of import in j.l.ClassLoader; these 
>>> were pushed by mistake (probably as a consequence of some automatic 
>>> IDE reordering). The idea is that we should try not to have 
>>> deliberate difference with upstream, as these will cause merge 
>>> failures (and imports is the #1 reason as to why Java code fails to 
>>> merge :-))
>>>
>>>
>>> Webrev:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8215289/
>>>
>>> Maurizio
>>>
>>>