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

Wed Sep 30 12:06:17 UTC 2020

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

Looks really good! Left some minor comments inline

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

> 153:  * features the corresponding {@link #HANDOFF} access mode.
> 154:  * <p>
> 155:  * For instance, if client wants to transfer ownership of a segment to another (known) thread, it can do so as
> follows:

Suggestion:

 * For instance, if a client wants to transfer ownership of a segment to another (known) thread, it can do so as follows:

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

> 175:  * the contents of the same memory segment concurrently (e.g. in the case of parallel processing). For instance, a
> client 176:  * might obtain a {@link Spliterator} from a shared segment, which can then be used to slice the segment
> and allow multiple 177:  * thread to work in parallel on disjoint segment slices. The following code can be used to sum
> all int values in a memory segment in parallel:

Suggestion:

 * threads to work in parallel on disjoint segment slices. The following code can be used to sum all int values in a
   memory segment in parallel:

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

> 425:      * The returned segment will feature only {@link MemorySegment#READ} and {@link MemorySegment#WRITE} access
> modes 426:      * (assuming these were available in the original segment). As such the returned segment cannot be
> closed directly 427:      * using {@link MemorySegment#close()} - but it will be closed indirectly when this native
> scope is closed. The

Suggestion:

     * using {@link MemorySegment#close()} - but it will be closed indirectly when the native scope is closed. The

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

> 428:      * returned segment will also be confined by the same thread as the provided native scope (see {@link
> NativeScope#ownerThread()}). 429:      * <p>
> 430:      * In case where the owner thread of the returned segment differs from that of this segment, write accesses to
> this

Suggestion:

     * In cases where the owner thread of the returned segment differs from that of this segment, write accesses to this

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

> 438:      * @throws IllegalStateException if this segment is not <em>alive</em>, or if access occurs from a thread
> other than the 439:      * thread owning this segment.
> 440:      * @throws UnsupportedOperationException if this segment does not support the {@link #HANDOFF} access mode.

This needs to be added to the other handoff overload as well.

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

> 874:
> 875:     /**
> 876:      * Handoff access mode; this segment support temporal bound changes (see {@link #handoff(NativeScope)} and

Suggestion:

     * Handoff access mode; this segment supports temporal bound changes (see {@link #handoff(NativeScope)} and

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

> 911:         /**
> 912:          * Creates a new shared handoff transform. The target segment produced by the returned transform is a
> shared 913:          * segment, which can be accessible concurrently from multiple threads.

Suggestion:

         * segment, which can be accessed concurrently from multiple threads.

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

> 920:         /**
> 921:          * Creates a new confined handoff transform, bound by a given thread. The target segment produced by the
> returned transform 922:          * is a confinement segment, which will only be accessible to a specific thread.

Suggestion:

         * is a confined segment, which will only be accessible by a specific thread.

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

> 932:         /**
> 933:          * Creates a new confined handoff transform, bound by the current thread (see {@link
> Thread#currentThread()}). 934:          * The target segment produced by the returned transform is a confinement
> segment, which will only be accessible to a specific thread.

Suggestion:

         * The target segment produced by the returned transform is a confined segment, which will only be accessible by the
           current thread.

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

> 945:          * segment produced by this transform will feature a cleanup action which will first call the
> user-provided action, 946:          * before delegating back to cleanup action associated with the source segment.
> 947:          * Any errors and/or exceptions thrown by the user-provided action will be discarded, and will not prevent
> the

Missing `<p>` here?

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

> 956:         /**
> 957:          * Specifies an additional attachment object which will be kept alive by the target segment, in addition
> 958:          * to any attachment object which might also be associated with the source segment.

Suggestion:

         * to any attachment object(s) which might also be associated with the source segment.

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

> 957:          * Specifies an additional attachment object which will be kept alive by the target segment, in addition
> 958:          * to any attachment object which might also be associated with the source segment.
> 959:          * This can be useful in cases where the lifecycle of the segment is dependent on that of some other
> external resource.

This doesn't make sense to me really. Isn't it more the other way around? I.e. the lifecycle of an external resource
should be dependent on the lifecycle of the target segment?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/NativeScope.java line 52:

> 50:  * returned by the native scope will be backed by memory segments confined by the same owner thread as the native
> scope. 51:  * <p>
> 52:  * To allow for more usability, it is possible for a native scope to reclaim ownership of an existing memory
> segments

Suggestion:

 * To allow for more usability, it is possible for a native scope to reclaim ownership of an existing memory segment

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/NativeScope.java line 37:

> 35: import java.nio.ByteOrder;
> 36: import java.util.OptionalLong;
> 37: import java.util.function.Consumer;

Seems to be an unused import?

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

> 407:      * hand-over from the current owner thread to the new owner thread, which in turn <i>happens before</i> read
> accesses 408:      * to the returned segment's contents on the new owner thread.
> 409:      *

Maybe it's worth mentioning here and for the other overload that cleanup actions are not run when doing a handoff, and
they are instead transferred to the new segment. While Cleaners registered to the old segment will have no effect
anymore, and will have to be re-registered to the new segment.

test/jdk/java/foreign/TestNative.java line 176:

> 174:         MemoryAddress addr = allocate(12);
> 175:         MemorySegment mallocSegment = addr.asSegmentRestricted(12)
> 176:                 .handoff(HandoffTransform.ofShared().addCleanupAction(() -> free(addr)));

Should this really be using ofShared() ? We don't have a withOwnerThread(null) currently either., and the current
asSegmentRestricted code ends up making a confined scope as well.

test/jdk/java/foreign/TestNative.java line 194:

> 192:         MemoryAddress addr = allocate(12);
> 193:         MemorySegment mallocSegment = addr.asSegmentRestricted(12)
> 194:                 .handoff(HandoffTransform.ofShared().addCleanupAction(() -> free(addr)));

Same here

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

Marked as reviewed by jvernee (Committer).

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