RFC: Refactoring SystemABI

[Henry Jen]

This looks good, I tried to have bound MH before, but because of that test, I decided to keep it as is as I thought that’s what we wanted.

By adding the UpcallInvoker, we basically achieve the same thing for Java code invoke callback from Java land, but that add cost to every NativeInvoker allocation, perhaps it should remains in CallbackImplGenerator as I expect this will only be used by binder?

Cheers,
Henry


> On Nov 23, 2018, at 8:53 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> For the records, I took the liberty to put together a patch which illustrates how we can get rid of dependencies on receiver object. The patch is based on current tip, and does not include other SystemABI changes (from Henry) - so that the actual differences will be more visible.
> 
> With this, UpcallHandler doesn't have much of an API. The only two methods used from outside are getNativeEntryPoint() [called immediately after creation] and free(). If, as discussed we move free() in SystemABI, then we can model upcalls with just a Pointer<?>. Cool!
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/upcall-recv-removal/
> 
> (I had to comment a bunch of lines in a test that was checking for == equality on functional interface objects)
> 
> Maurizio
> 
> On 23/11/2018 12:19, Maurizio Cimadamore wrote:
>> (as also observed by Jorn), Pulling even more on this string, I think we can drop receiver object from the getUpcallHandle method alltogether.
>> 
>> The current API looks like this:
>> 
>> UpcallHandle upcallStub(Object receiver, MethodHandle target, NativeMethodType nmt);
>> 
>> I think we can simplify to this:
>> 
>> UpcallHandle upcallStub(MethodHandle target, NativeMethodType nmt);
>> 
>> 
>> In fact, the method already takes a MethodHandle - so we could require clients to pass in a 'bound' method handle, where the receiver object has already been 'attached' to the target MH. This will also make the SystemABI more flexible too, because, while in the case of the binder there is _always_ a receiver object (the receiver is an instance of a functional interface obtained from some lambda expression passed to the Scope::allocateCallback), in general this need not be the case - and one could envision cases where clients would want a callback to point to some static method with the right signature.
>> 
>> So, I think handling the receiver on the binder side (by binding it to the MH) is the right thing to do.
>> 
>> So, with all the simplifications discussed in this thread, the API would become:
>> 
>> Pointer<?> upcallStub(MethodHandle target, NativeMethodType nmt);
>> 
>> That is, gone is the UpcallHandle abstraction.
>> 
>> Additionally, we will have to add a new method to SystemABI:
>> 
>> void freeUpcallStub(Pointer<?>)
>> 
>> Which is used by Scope::close.
>> 
>> 
>> As mentioned in an earlier message, the downcall logic in SystemABI will have to special case the case where a stub entry point is passed. In such cases, the ABI will recover the method handle associated with the stub (we currently do so by saving a Java object in the stub itself - but other ways are possible too, e.g. global hashmap), and return the (already bound) method handle, which will already feature the right signature.
>> 
>> 
>> This will eliminate code paths duplication in CallbackImplGenerator and also CallbackImpl - where currently we do different things depending on whether the underlying pointer is a stub.
>> 
>> I think this looks good?
>> 
>> Maurizio
>> 
>> 
>> On 23/11/2018 02:06, Maurizio Cimadamore wrote:
>>> 
>>> On 23/11/2018 01:47, Maurizio Cimadamore wrote:
>>>> What is it that bothers you about this approach? 
>>> 
>>> Let me clarify - I like that you are thinking of ways to reduce the surface of the API - but it seems that moving a method (getReceiver) from UpcallHandle to SystemABI doesn't buy all that much.
>>> 
>>> I personally prefer having a simple UpcallHandle lookup method in the ABI, and then a way to get form the Handle to the receiver as it seems more composable this way. Right now, true, we always need to do both things at the same time (to speed up things in the upcall machinery), but maybe having a 'are you a stub' predicate will turn out handy even on its own (in case the user wants to write some pointer-walking code).
>>> 
>>> But, if we really wanted to save some API estate, I think a better approach (if viable) would be to completely hide the stub detection logic. As you said very clearly yourself, we have this issue:
>>> 
>>> "There is a shortcut being taken when native code gives us a Pointer back to one of our upcall stubs, and we want to call it from Java. In that case we don't really need to call into native, to call our own upcall stub to then call back into Java to call our target method."
>>> 
>>> This is the reason behind the 'isStub/getReceiver' logic. But let's analyze this use case more in detail: we have a pointer to function, and we want to do a downcall using that pointer as the entry point. But this means that, in order to do the downcall, we will call SystemABI.downcallHandle (again). So, I think it is theoretically possible that the ABI would say: "hey, I know this guy!" and instead of giving you a MethodHandle that 'goes down' it gives you a MethodHandle that stays in Java-land. And the binder would not even know.
>>> 
>>> If my conjecture is correct, then we just need your minimal UpcallHandle interface (pointer + free) and no getReceiver/getFunctorObject at all. And, if you pull on this some more, you realize that UpcallHandle is not even needed: just use a Pointer, and add a 'freeUpcallHandle(Pointer<?>)' to the SystemABI. That'd be sweet.
>>> 
>>> Thoughts?
>>> 
>>> Maurizio
>>>