RFC: Refactoring SystemABI

Fri Dec 7 11:45:43 UTC 2018

I'm lost :-(

I think we agreed to use Symbol in both upcall/downcall methods...

Having downcall handle accept just any pointer is IMHO a step back. It's 
much easier to shoot yourself in the foot. Symbol was fine here.

We also discussed how, for consistency, upcallStub needed to return a 
symbol too - if internally you want to call it JavaSymbol, fine - but I 
think from a public API perspective the two methods should be:

public MethodHandle downcallHandle(CallingConvention cc, Symbol s, NativeMethodType nmt);
public Symbol downcallHandle(CallingConvention cc, MethodHandle target, NativeMethodType nmt);

Let's also try to keep this simple - putting the MethodHandle logic in 
Symbol is something that might have some value, but (1) is non essential 
(given the above API you can always get a MethodHandle for a Symbol 
using SystemABI::downcallHandle) and (2) has not been discussed in this 
thread.

Also, we should try to reduce the surface area in terms of public API 
and this patch seems to go in the direction of adding two new types (not 
yet public but...): JavaSymbol and JavaExports, both of which I don't 
think pass the test for 'need to be in public API'. I think a lot of the 
rationale behind these classes is to avoid creating a Callback wrapper 
in cases where the underlying symbol is coming from java - and I think 
it's not worth making the API more complex to solve a problem that never 
comes up in practice (yes, you can call the callback stub you have 
created from Java, but in reality, how frequent that would be?).

In other words:

1) SystemABI::upcallStub returns a Symbol
2) CallbackImpl saves that symbol somewhere
3) CallbackImpl::asFunction creates a callback wrapper (using 
CallbackImplGenerator) and pass it the symbol it has
4) The implementation generated by CallbackImplGenerator will take care 
to do the downcall using SystemABI (passing the symbol). There will be 
two cases here:
4a) the Symbol is the one obtained from SystemABI::upcallHandle - in 
that case the downcall logic will do something special and bypass the 
native call
4b) the Symbol is some opaque entry point created by the binder (e.g. 
when reading a function pointer field in a struct); in that case a full 
downcall happens

I think this is self-consistent, and does not require any extra API. 
What's better, we completely delegate to the ABI the task of recognizing 
'its own' upcall symbols, which means that each ABI implementation could 
have a different notion of upcall symbols - and some ABIs might even 
decide not to optimize (4a) and always go native.

Also, please no Kind on NativeMethodType - this should be about types - 
and the new kind turns it more into a kind of 'method handle'.

To summarize, it feels (probably because of the back and forth made 
during the discussion last week, and that's my fault too!) that a bad 
turn has been made somewhere. I suggest we go back to a simpler state 
and simply define the ABI upcall/downcall methods with the signatures 
shown above.

Thanks - I know this feedback will probably sound harsh, and I also know 
you put a lot of effort into this; but we need this API to settle into a 
low energy state where SystemABI only depends on a bunch of public types 
(Pointer, Symbol, NativeMethodType, LayoutType). I think this is crucial 
to get this API right - the more API points we introduce, the more 
dependencies between the ABI and the binder, and we want to keep those 
to a minimum!

Maurizio

On 07/12/2018 08:03, Henry Jen wrote:
> Please have a look at updated webrev.02[1]
>
> Quick summary:
>
> 1. SystemABI upcall/downcall are dealing with Pointer, not Symbol
> 2. abi.sysv.x64 package is abi.x64.sysv as suggested by Jorn
> 3. Library.Symbol becomes the public API presenting a consistent view for native and java symbol. From which, one can get  MH for java call and Pointer for native function pointer.
> 4. I haven’t move invokers into abi specific folder as I think most of them would be share code, let me know particular file you think should be moving, we can always do that later though.
>
> There are still thing to improve, but the Symbol approach come together nicely. I haven’t expose JavaSymbol just yet, it is now to be managed via Scope/Callback which manage the life-cycle.
>
> In a way, JavaSymbol is the UpcallHandle we had earlier.
>
> JavaExports was created to be the public API to obtain JavaSymbol, with API for each form of JavaSymbol construction with unbounded pointer and user will need to manage the lifecycle of the symbol. I moved them back into as JavaSymbol.of(..). If we like this, then we can open it up.
>
> It currently serve the global registry of native address to JavaSymbol lookup, which I am debating whether should be move into CallbackImpl.
>
> Let me know what you think. There are still some cleanup can be done, also if we like the Symbol approach, we should add some test cases.
>
> Cheers,
> Henry
>
> [1] https://cr.openjdk.java.net/~henryjen/panama/SystemABI/webrev.02/webrev/
>
>
>
>> On Dec 4, 2018, at 10:43 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>
>>
>> On 04/12/2018 15:51, Jorn Vernee wrote:
>>> Maybe this concern is being raised prematurely. I think we can see how the current plans pans out and then look at this again if needed.
>>>
>> Yes, let's revisit this later. I have a couple of different ideas on how to get there, but it doesn't make a lot of sense to discuss them here, as it's probably too early - and I don't want to add to the confusion.
>>
>> To recap (for Henry): let's have SystemABI::upcallStub return a Symbol and take it from there.
>>
>> Thanks for your comments, as usual.
>>
>> Maurizio
>>
>>> Jorn
>>>
>>> Jorn Vernee schreef op 2018-12-04 16:34:
>>>> Maurizio Cimadamore schreef op 2018-12-04 16:07:
>>>>> On 04/12/2018 14:23, Jorn Vernee wrote:
>>>>>> We have Scope::allocateCallback to replace step 2. and 3., but that requires an annotated functional interface object. What if a user just has a MethodHandle?
>>>>> Not sure I follow - a Callback is based on the idea that you _do have_
>>>>> a functional interface.
>>>> The scenario I'm talking about still uses a functional interface
>>>> _type_. But the Scope::allocateCallback api expects _an instance_ of
>>>> such a functional interface type; So if you just have a MethodHandle
>>>> you would first have to turn it into an instance of this functional
>>>> interface type and then call Scope::allocateCallback. This adds a
>>>> level of indirection, and AFAIK there's no straight forward API for
>>>> wrapping a MethodHandle in a functional interface in the JDK
>>>> (MethodHandleProxies comes close but it relies on reflection, so it's
>>>> slow).
>>>>
>>>> So what I'm talking about is that a user can use System::upcallStub to
>>>> make the MethodHandle callable from native, but if they have some
>>>> other method that accepts a Callback there still needs to be a way to
>>>> convert the value returned from upcallStub into a Callback.
>>>>
>>>>> I don't think it makes sense to mix the abstraction levels - the
>>>>> binder will represent all callbacks using Callback, so I don't expect
>>>>> a user of the high-level binder to mess with SystemABI interface to
>>>>> allocate stubs.
>>>> I was thinking about the inverse; we have a user who is using the
>>>> low-level SystemABI, and wants to have the high-level addition of
>>>> Callback. This also makes sense where low-level code has to interact
>>>> with high-level code.
>>>>
>>>> Jorn