RFC: Refactoring SystemABI

Tue Dec 11 11:10:39 UTC 2018

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