RFC: Refactoring SystemABI

[Henry Jen]

I pushed webrev.05 as is. Thanks all the review and I think we have some good discussion to get here.

This is indeed an issue we should revisit. On one hand, the code path should only happen when a Java function passed to native call as function pointer and somehow later be passed back into Java, thus it should have exactly same native type as the native function pointer should have. OTOH, the only way to get a Java MH is from a native function pointer is end up calling SystemABI.downcall().

I can understand we may want to use a different type on the Java side, which should be achievable with MH.asType. But we should revisit this.

Cheers,
Henry


> On Dec 11, 2018, at 3:10 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> And, one thing I forgot; I realized that there's a bug (also in the current impl, not just in your patch): always reusing the method handle from an UpcallHandle is not correct! Note that the UpcallHandle has been created with a given NativeMethodType - when you pass that UpcallHandle to SystemABI::downcallHandle, then you can provide a different NativeMethodType, in which case you *could* have mismatches.
> 
> At the very least we should keep track of the NMT and check if it's the same before recycling the MH. But it feels like we are in a game of diminishing returns here. Yes, there is some potential to optimize that, but it's not as straightforward as it seems.
> 
> Maurizio
> 
> 
> On 11/12/2018 11:03, Maurizio Cimadamore wrote:
>> This looks good, thanks.
>> 
>> As for the package, whether invokers should be in abi or abi.sys5, I think the jury is out, and we'll wait to see how other ports will go. Probably UniversalInvoker can be shared, whereas the DIrect one is likely to be Windows specific (but when we'll have LinkToNative, then we will be able to drop Direct and replace it with that which should work better across platforms).
>> 
>> Maurizio
>> 
>> On 11/12/2018 06:35, Henry Jen wrote:
>>> Updated webrev.05[1]. invokers are now in abi. Removed JavaSymbol.
>>> 
>>> [1] https://cr.openjdk.java.net/~henryjen/panama/SystemABI/webrev.05/webrev/
>>> 
>>> Cheers,
>>> Henry
>>> 
>>>> On Dec 10, 2018, at 4:47 PM, Henry Jen <henry.jen at oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Dec 10, 2018, at 4:06 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>> 
>>>>> Hi,
>>>>> I think this is a lot closer to what I had in mind.
>>>>> 
>>>>> If I'm correct there are only two usages of JavaSymbol:
>>>>> 
>>>>> * ScopeImpl creates one using JavaSymbol::exportTo
>>>>> * Scope stores them into a List, and then frees them when the scope is closed
>>>>> 
>>>>> I think that, JavaSymbol serves little purpose now, and can be removed. You can just call the ABI code directly to do whatever JavaSymbol::exportTo does, and, similarly, you can do the same for closing.
>>>>> 
>>>> True. In that case, we pretty much back to .01. JavaSymbol is just a higher level construct to give a better symbol name and correct Pointer scope. But we don’t really need it since we only use pointer address to get back the UpcallStub.
>>>> 
>>>>> Your new UpcallStub is what I really would call JavaSymbol - and this type is entirely under the water - it's completely ABI specific (and should live in the 'abi' package). So, I think it is better, but the improvement is somewhat hidden by the fact that the 'invokers' live in their own separate package, while they should really live in the 'abi' package (in fact the binder doesn't 'need' them anymore).
>>>>> 
>>>> ABI specific means SysV specific or ABI in common? That decided where that should go. Move .invokers into .abi seems reasonable to me. JavaSymbol to me need to have real name of Java class/method name. :)
>>>> 
>>>> Cheers,
>>>> Henry
>>>> 
>>>> 
>>>>> Other than that I think it looks good?
>>>>> 
>>>>> Maurizio
>>>>> 
>>>>> On 10/12/2018 23:50, Henry Jen wrote:
>>>>>> I took another hack to push down the search down into SysVx64ABI so that ABI implemetnation is not to be aware to JavaSymbol, but in order to have MethodHandle in CodeBlob, we sort of have UpcallHandle back.
>>>>>> 
>>>>>> Take a look at webrev.04[1] or the diff[2] between .03 and .04, not sure .04 is much better. After all, JavaSymbol is internal although it is indeed a level above ABI and have SysVx64ABI be aware of it doesn’t feel “clean”.
>>>>>> 
>>>>>> Let me know what you think. At this point, I kind of think it better we pushed either .03 or .04, at least we have SystemABI interface finalized.
>>>>>> 
>>>>>> Cheers,
>>>>>> Henry
>>>>>> 
>>>>>> [1] http://cr.openjdk.java.net/~henryjen/panama/SystemABI/webrev.04/webrev/ 
>>>>>> [2] http://cr.openjdk.java.net/~henryjen/panama/SystemABI/diff-03-04/webrev/ 
>>>>>> 
>>>>>> 
>>>>>>> On Dec 10, 2018, at 8:58 AM, Henry Jen <henry.jen at oracle.com> wrote:
>>>>>>> 
>>>>>>>> On Dec 10, 2018, at 3:16 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> This is looking better thanks.
>>>>>>>> 
>>>>>>>> I think JavaSymbol is still doing more than it should, and a sign of that is the fact that now you need an hash table to keep track of the allocated stubs in order to find them. This is a new requirement that was not present in the old code, and is introduced by the fact that, I think, we are trying to look 'too hard' for whether a pointer corresponds to an existing JavaSymbol.
>>>>>>>> 
>>>>>>> Yes and No. There are two thing I am not satisfied with this patch, one is the map used to find JavaSymbol from a Pointer and the other is we have to save a copy of Library.Symbol returned from SystemABI for free later.
>>>>>>> 
>>>>>>> The first dislike, as you have explained later, sort of duplicate to native side where we can search for a CodeBlob. I think we can push that down to SysVx64ABI, but not sure how much that search will be better while we can do it earlier.
>>>>>>> 
>>>>>>> Now, from the pointer to MH, you either need a NMT to have SystemABI make it, or trying to “stay above” by getting an UpcallHandle, this is basically now a JavaSymbol.
>>>>>>> 
>>>>>>> The extra stub, well, we need to wrap up the pointer with socpe. But SystemABI upcallHandle now return a symbol, we should not be assuming any symbol with same pointer can do, so we have to save it for free later.
>>>>>>> 
>>>>>>> Rest of code in JavaSymbol, we will have to do the same for ScopeImpl, either in JavaSymbol or somewhere, JavaSymbol seems to give a good sensible place for those.
>>>>>>> 
>>>>>>> Adding a Kind enum and a instanceof operation, I don’t see the benefit. Personally, I think current approach is good, and perhaps when we open up with other language, we could have NashornSymbol or JRubySymbol.
>>>>>>> 
>>>>>>> Make sense?
>>>>>>> 
>>>>>>> Cheers,
>>>>>>> Henry
>>>>>>>