RFC: Refactoring SystemABI

[Jorn Vernee]

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