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

[Paul Sandoz]

On Thu, 10 Sep 2020 17:58:01 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.

Nice utility.

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

> 277:
> 278:         /** Dummy cleanup action */
> 279:         CleanupAction DUMMY = new CleanupAction() {

Recommend renaming to `NO_ACTION`

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

> 294:      * guarantees this invariant even when multiple threads race to call the {@link #cleanup()} method.
> 295:      */
> 296:     static abstract class BasicCleanupAction implements CleanupAction {

Rename to `AtMostOnceCleanupAction` or some other snappier name that conveys it's purpose. Embedded in `CleanupAction`
then `CleanupAction.AtMostOnceOnly` might work.

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

> 333:                 // reachability fences which prevent a scope to be deemed unreachable before we are done
> 334:                 // marking the original cleanup action as "dead".
> 335:                 throw new IllegalStateException("Already cleaned");

I recommend not throwing, it may cause noise for the developer when they observe through say flags or events that
exceptions are being thrown.

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

> 321:                 @Override
> 322:                 void run() {
> 323:                     BasicCleanupAction.this.run();

This has the effect of chaining calls, that might accumulate over time. I don't see how to avoid this unless a
delegating model is used. But IIUC you want to avoid creating two objects, the "clean up" action as a `Runnable` then
the "scope close action" encapsulating the "clean up" action. Perhaps when inline types are available we can do better.

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

> 88:
> 89:     protected Object ref;
> 90:     protected CleanupAction cleanupAction;

Make `final`

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 303:

> 301:         checkValidState();
> 302:         MemoryScope.CleanupAction cleanupAction = scope.cleanupAction;
> 303:         cleaner.register(this.scope, cleanupAction::cleanup);

If `CleanupAction` extended from `Runnable` you could pass it in directly. e.g rename `cleanup` to `run`, and flip the
two methods on `BasicCleanupAction`

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

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