[foreign] RFR 8215289: Cleanup code post SystemABI integration

[Maurizio Cimadamore]

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