[foreign-memaccess] RFR: 8253025: Add support for registering a segment with a Cleaner [v2]

[Paul Sandoz]

On Fri, 11 Sep 2020 10:48:54 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch adds an extra API point to register a segment against a cleaner.
>> 
>> The basic idea is that, when a segment is registered against a cleaner, the segment's scope is register, and the
>> scope's cleanup action is passed to the cleaner.
>> In practice, things are more subtle than it seems. First, we need to make sure that we cannot have races - which could
>> result in cleanup action being called multiple times. The possible races are:
>> * close vs. cleaner
>> * cleaner vs. cleaner
>> 
>> We can get rid of the first type of race by adding reachability fences around the scope terminal operations. If we do
>> that, we are guaranteed that the Cleaner cannot kick in before we are actually done with the scope.
>> Now, even if reachability fences can give us some guarantee that close will never run concurrently with cleaner, we're
>> not out of the woods yet. The cleaner can in fact still be called _after_ a segment has been closed explicitly.
>> Now, the simplest thing to do on paper would be to check the scope liveness bit, and only call the cleanup action if
>> the scope is still alive. But this is not possible, becaue the scope liveness bit is, well, inside the scope and, to
>> access it, the Cleaner action must keep a strong reference to the scope being registered (which means the cleaner will
>> never work).  To address this, I came up with a level of indirection: I replaced the references to Runnable with a new
>> interface, called CleanupAction. This interface has methods to cleanup and to dup onto a new cleanup action, killing
>> the previous one.  Now, segment factories create cleanup action, rather than simple Runnable. There is a static "dummy"
>> cleanup action, which does nothing; and there is also a more complex and stateful cleanup action, which supports
>> atomicity (this cleanup action has a boolean flag which is chcked with CAS).  So, all terminal operation in memory
>> scope must either call cleanup() or dup() on the cleanup action, to ensure that the cleanup action attached to the
>> scope is no longer usable upon return.  Since the logic inside the stateful cleanup action uses CAS, it is possible for
>> multiple threads to race to cleanup the same action, and only one of them is guaranteed to win.  While the logic is
>> tricky, and partly replicates the liveness bit in scope, the implementation strategy tries hard not to allocate more
>> than previosly, especially when a new segment is created. The only drawback is that we need to create a new cleanup
>> action every time we change scope ownership - but these operations are not performance critical, so I assume this is an
>> acceptable cost.  Note that, given all our core memory access operation (in the newly define ScopeMemoryAccess) already
>> take care of adding the require reachability fence on the scopes being accessed, it is not possible for a cleaner to
>> kick in while a segment is being actively accessed.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Addressed review comments
>   Beefed up test

Marked as reviewed by psandoz (Committer).

test/jdk/java/foreign/TestCleaner.java line 97:

> 95:     enum SegmentFunction implements Function<MemorySegment, MemorySegment> {
> 96:         IDENTITY(Function.identity()),
> 97:         CLOSE(s -> { s.close(); return s; }),

Won't that just skip cleaner registration, making the test redundant for that case? I guess for this case it should be
called after registration?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 325:

> 323:             public CleanupAction dup() {
> 324:                 disable();
> 325:                 class DupAction extends AtMostOnceOnly {

Nice approach. Shame we cannot declare static classes within a method, since the inner class does not need to
implicitly capture "this" of the outer class. I suppose we could make it static for some marginal efficiency gain
reducing dependency chains.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/320