[foreign] RFR 8215553: Remove SystemABI::freeUpcallStub

[Henry Jen]

Looks fine.

Cheers,
Henry


> On Dec 18, 2018, at 9:57 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> 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
>>>>>>