[foreign] RFR 8215553: Remove SystemABI::freeUpcallStub

[Maurizio Cimadamore]

Updated webrev

cr.openjdk.java.net/~mcimadamore/panama/8215553_v2/

Cheers
Maurizio

On 18/12/2018 16:09, Maurizio Cimadamore wrote:
> Right - I will simplify.
>
> Thanks for the comments
>
> Maurizio
>
> On 18/12/2018 16:05, Henry Jen wrote:
>> Just curious, why use a Supplier in registerUpcallStub? Simply 
>> because to get the return type?
>>
>> Cheers,
>> Henry
>>
>>> On Dec 18, 2018, at 5:52 AM, Sundararajan Athijegannathan 
>>> <sundararajan.athijegannathan at oracle.com> wrote:
>>>
>>> File: 
>>> src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVx64ABI.java
>>> Line: + cleaner.register(up, () -> freeUpcallStub(ptr));
>>>
>>> This could be
>>>
>>>       cleaner.register(up, () -> freeUpcallStub(ptr.addr()));
>>>
>>> which will avoid capturing "this" object in the cleaner lambda - 
>>> because freeUpcallStub(long) is a static method whereas 
>>> freeUpcallStub(Pointer) is an instance method. Or you could make 
>>> freeUpcallStub(Pointer) method as static.
>>>
>>> -Sundar
>>>
>>> On 18/12/18, 6:44 PM, Maurizio Cimadamore wrote:
>>>> Forgot to add: I tested the patch by running all tests multiple 
>>>> times (esp. the heavy TestUpcall :-)) and adding println statements 
>>>> when the cleaner was called, to make sure that the logic was indeed 
>>>> running when executing long-lived tests.
>>>>
>>>> Maurizio
>>>>
>>>> On 18/12/2018 13:12, Maurizio Cimadamore wrote:
>>>>> Hi,
>>>>> few months ago we moved to a new model for managing lifecycle of 
>>>>> callbacks, where callbacks were explicitly allocated through a 
>>>>> Scope; when the scope is closed, all callbacks registered onto 
>>>>> that scope are freed.
>>>>>
>>>>> Back then, I failed to realize that we don't really need an API 
>>>>> point to free the callback - we can just rely on a Cleaner to do 
>>>>> its job when the callback is no longer referenced from Java code. 
>>>>> Since a callback will always be stored in some scope (when 
>>>>> active), this will effectively prevent it from being collected.
>>>>>
>>>>> Note: this model is very different from what we had before, where 
>>>>> callbacks were NOT allocated using a Scope, but only implicitly by 
>>>>> passing lambdas to functions. In that case, using a Cleaner was 
>>>>> bad, as it  sometimes led to GC happening _in the middle_ of a 
>>>>> function call. This can no longer happen, as a Scope will now hold 
>>>>> tight on the callbacks used at any given point in time.
>>>>>
>>>>> Of course, users of the low-level SystemABI, must pay extra care: 
>>>>> as soon as an upcall symbol goes out of scope, it is ready for 
>>>>> collection; that means that if a client of SystemABI wants an 
>>>>> upcall symbol to remain alive, it has to stash it somewhere. Given 
>>>>> the audience of this SystemABI API, I think this is an acceptable 
>>>>> compromise (and one I've seen elsewhere, e.g. JNR). On the plus 
>>>>> side, the API got tighter, as we no longer need a 'free' method.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8215553/
>>>>>
>>>>> Cheers
>>>>> Maurizio
>>>>>