RFC: Refactoring SystemABI

[Maurizio Cimadamore]

What you describe is possible, but requires a field access/method call 
on every upcall (to get the method handle), plus I'm not sure how we can 
effectively invoke a MH from native code.

I think a simple solution would be to just use some #ifdef ?

It's not the most elegant solution in the world, I know, but after all 
we'll only be using one SystemABI in a given system.

Another way would/could be to invoke a virtual method on some 
UniversalUpcallHandler superclass which is extended by SysV and Windows. 
After all the native code doing the upcall does have a receiver, so we 
could use it.

Maurizio

On 23/11/2018 12:55, Jorn Vernee wrote:
> Looks good!
>
> Now let's talk about the upcall stub a bit;
>
> If there are 2 UniversalUpcallHandlers (in separate ABI 
> implementations), that want to use the same upcall stub generation 
> code this breaks currently, since the upcall stub generation code will 
> always call the SysV implementation. This is the current code [1]:
>
>   const char* cname = 
> "jdk/internal/foreign/invokers/UniversalUpcallHandler";
>   const char* mname = "invoke";
>   const char* mdesc = 
> "(Ljdk/internal/foreign/invokers/UniversalUpcallHandler;JJJJJJ)V";
>
> But after moving the UniversalUpcallHandler to the SysV impl package 
> this will always call the the SysV implementation.
>
> I think this coupling could be reduced a bit. Instead of saving the 
> UpcallHandler in the stub, which will hold a reference to the target 
> MethodHandle , we could instead save the target MethodHandle in the 
> stub, for later retrieval, and also save a perfixed MethodHandle which 
> calls the right UpcallHandler. Then, instead of the upcall stub 
> calling into the UniversalUpcallHandler.invoke method, it would just 
> call this prefixed MethodHandle instead.
>
> Thoughts?
>
> Jorn
>
> [1] : 
> http://hg.openjdk.java.net/panama/dev/file/9f27bcd3de9c/src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp#l59
>
> Maurizio Cimadamore schreef op 2018-11-23 13:19:
>> (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
>>>