[foreign-abi] RFR: 8253823: Investigate ways to make handoff-like operation more explicit [v2]

[Paul Sandoz]

On Tue, 29 Sep 2020 22:09:32 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> The API currently doesn't do a good job at differentiating between simple wither-like methods (e.g. withAccessModes)
>> and deep side-effecting operations (e.g. withOwnerThread).
>> This patch explores a way to make handoff more of a first-class concept of the API - you can take apart an existing
>> segment and reconstruct it with a given `HandoffTransform` (a recipe which specifies what properties should be
>> associated with the reconstructed segment). Turns out that `handoff` and `HandoffTransform` is the ideal place where to
>> put several of the *advanced* operations we used to have exposed directly by the memory segment API
>> (`withCleanupAction`, `registerCleaner`). We can also add na overload for `handoff` which takes a native scope and
>> bounds the segment to it.  By reorganizing the API in this way, it is now clear that memory segment feature _two_
>> terminal operations: `close` and `handoff`. We can document this in the API in a more centralized fashion, without
>> having to repeat the same text across seemingly unrelated methods. For clients, it is also clearer that doing an
>> `handoff` is a big reconstruction, which is different from a simple wither.  I've also somewhat simplified the access
>> mode story; there's now a single HANDOFF mode which covers all types of handoff - this was clearly needed, since the
>> access modes we had before were very fine-grained, and it was very painful for client to prevent against *all* kinds of
>> terminal operations (for instance, I realized that the current impl has no access modes check for
>> `withCleanupAction`).  Impl note: for now the HandoffTransform implementation is mutable and not thread-safe - the
>> javadoc calls this out. I'm open to make this more immutable/value oriented if this is desirable.  Note: this PR is
>> against the `foreign-abi` so that it shows how `NativeScope` is affected by the changes. If the changes brought in this
>> patch looks good, it would be better to push the memory access changes in the `memory-access` branch in a separate PR,
>> and then follow up with the remaining `NativeScope` changes here.  Javadoc  link:
>> http://cr.openjdk.java.net/~mcimadamore/panama/segment-rebuilder-javadoc_v2/javadoc/jdk/incubator/foreign/package-summary.html
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert commented lines in StdLibTest

I think this provides much more clarity with a minimal increment in API surface.

Unsure if `MemorySegment` should closely couple with `NativeScope`, if there is an intention to separate out into base
and/or other modules. Perhaps  `NativeScope` is independently useful from the ABI that it should be pushed down into
the memory-access branch and part of the core memory API?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 909:

> 907:      * @see MemorySegment#handoff(HandoffTransform)
> 908:      */
> 909:     interface HandoffTransform {

There might be a use-case for starting out with the existing confinement properties of a segment, say if just
registering a cleaner or adding an attachment e.g. `HandoffTransform.ofSegment`, which is not a good name since it
would imply the acceptance of a segment parameter, whereas it would be applied to the segment that accepts the handoff
transform. That way it's not necessary to query a segment before constructing the correct transform.

I suspect there will only be one implementation of `HandoffTransform`, since it's really just a dumb holder of
accumulated state, so it could be made into a `final class` (value-based) with a non-public constructor.

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

> 315:
> 316:         final Thread ownerThread;
> 317:         List<Runnable> cleanupActions = new ArrayList<>();

Mark all fields as `final`

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 150:

> 148:  * The {@link #handoff(HandoffTransform)} method can be used to change the thread-confinement properties (as well
> 149:  * as other properties) of a memory segment. This method is, like {{@link #close()}} a <em>terminal operation</em>
> 150:  * which marks the original segment as not alive (see {@link #isAlive()} and create a <em>new</em> segment with the

s/create/creates

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

Changes requested by psandoz (Committer).

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