[foreign-memaccess+abi] RFR: 8263018: Improve API for lifecycle of native resources [v10]

Paul Sandoz psandoz at openjdk.java.net
Fri Mar 12 01:32:25 UTC 2021 

On Thu, 11 Mar 2021 11:53:35 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch brings the API improvements described in [1]. Note that this is a significant reshuffle of the API, and might have non-trivial source compatibility impact; the main moves are listed below:
>> 
>> * MemorySegment is no longer AutoCloseable (ResourceScope is, and a MemorySegment *has a* ResourceScope)
>> 
>> * Resources created without an explicit scope (e.g. `MemorySegment::allocateNative(long)`) get a *default* scope, which is non-closeable and GC managed. In other words, users who do not bother with resource scopes, will get same functionalities (lifecycle-wise) that they get with the ByteBuffer API
>> 
>> * Support for custom allocators is added via the `SegmentAllocator` interface; now all API points which need to perform some allocation will accept an explicit `SegmentAllocator` parameter. Where SegmentAllocator-less overloads are provided, the assumed semantics is that of `MemorySegment::allocateNative(long, long)` which means the returned segments will be associated with the *default scope* and will **not** be closeable.
>> 
>> * The method handles generated by the linker will accept an additional leading parameter of type `SegmentAllocator` whenever the native function returns a struct by value. There is an overload which allows to statically bind an allocator at MH creation time.
>> 
>> * The NativeScope abstraction has been removed. This follows the observation that with `SegmentAllocator::arenaBounded/Unbounded` clients can already get pretty close to the functionalities provided by a NativeScope (that said, at least initially, it is likely that jextract will emit a NativeScope abstraction in the generated code, to minimize compatibility impact).
>> 
>> * As discussed in [1], the new API has a way to prevent a resource scope from being closed when operating on a resource associated with that scope; this is `ResourceScope::acquire` which returns a so called *resource scope handle* (an AutoCloseable).
>> 
>> * Access modes are gone. We only kept read-only views (as they are needed to support mapped memory). Non-closeable segments can be obtained by using the acquire API.
>> 
>> * Several methods in MemorySegment are also gone; e.g. all methods related to ownership changes (`withOwnerThread`, `share`) as well as some of the predicate methdos (e.g. `ownerThread`, which is now available through the segment's scope). The overall philosophy is that a scope is assigned to a segment on creation; the scope contains details about how the segment is to be accessed, and these details cannot be altered after the fact.
>> 
>> * `MemoryAddress::asSegmentRestricted` now takes an optional ResourceScope parameter - the scope to be associated with the returned (unsafe) segment. There is also an overload that doesn't take a ResourceScope, in which case the *global scope* (singleton, non-closeable scope) will be used. A similar argument applies to `VaList.ofAddressRestricted`.
>> 
>> * The linker will now accept a scope for the returned upcall stub segment - if no scope is provided, a default one (GC-managed, non-closeable) will be created instead. Same for `VaList::make`.
>> 
>> Overall, I think this patch makes the API cleaner by clarifying the role between lifcycles (e.g. resource scopes) and resources (segments, va lists, etc.) , w/o making clients much more verbose. We also did some internal validation (thanks Chris) to make sure that async byte buffer operation could be adjusted using the *acquire* mechanism. There are some minor outstanding issues which will need to be resolved (as part of this PR, or as a followup) - listed below:
>> 
>> * should the default scope accept custom cleanup actions? Since this scope is created internally using our cleaner factory, I think there's a possibility that user-defined cleanup action might stall the cleaner forever.
>> 
>> * should ResourceScope have a predicate to see if it's a default scope? How should it be called? In an earlier iteration I had `isCloseable` which I don't think does a good job (as a scope can also not be closeable for different reasons).
>> 
>> * Are we ok with the ResourceScope::acquire/ResourceScope.Handle names? Also note that the latter is a simple AutoCloseable, but we need a subclass because the main AutoCloseable::close throws Throwable. Which is unfortunate.
>> 
>> * What about NativeScope? Are we ok with *not* having it? Note that, without NativeScope, code like:
>> 
>> try (NativeScope scope = NativeScope.ofUnbounded()) {
>>    ...
>> }
>> 
>> becomes:
>> 
>> try (ResourceScope scope = ResourceScope.ofConfined()) {
>>    SegmentAllocator allocator = SegmentAllocator.arenaUnbounded(scope);
>>    ...
>> }
>> 
>> E.g. one extra line in the user code. I believe this is not a huge deal, in the sense that in all our example (most of which are jextract based), the body of the try with resources if pretty biggie in comparison. That said, I'm open to reintroduce NativeScope - although probably in a different form - e.g. by having a sub-interface of SegmentAllocator called BoundedAllocator, which is essentially an allocator that *has a* scope. But we can also add this later depending on our experience with using the API.
>> 
>> That's all for now. Feedback welcome!
>> 
>> [1] - https://inside.java/2021/01/25/memory-access-pulling-all-the-threads/
>
> Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Remove spurious println
>  - Clarify javadoc for SegmentAllocator::arenaUnbounded

Very nice. A good improvement, with modest disruption (based on changes to the tests). Just some minor comments.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 185:

> 183:      * type's return type is {@code MemorySegment}, then the resulting method handle features an additional
> 184:      * prefix parameter (inserted immediately after the address parameter), of type {@link SegmentAllocator}),
> 185:      * which will be used by the linker runtime to allocate structs returned by-value.

Suggestion:

    /**
     * Obtains a foreign method handle, with the given type and featuring the given function descriptor, which can be
     * used to call a target foreign function at an address.
     * The resulting method handle features a prefix parameter (as the first parameter) corresponding to the address, of
     * type {@link Addressable}.
     * <p>
     * If the provided method type's return type is {@code MemorySegment}, then the resulting method handle features an
     * additional prefix parameter (inserted immediately after the address parameter), of type {@link SegmentAllocator}),
     * which will be used by the linker runtime to allocate structs returned by-value.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 132:

> 130: 
> 131:     /**
> 132:      * Obtain a foreign method handle, with the given type and featuring the given function descriptor,

Suggestion:

     * Obtains a foreign method handle, with the given type and featuring the given function descriptor,

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

> 408:      * Copy the contents of this memory segment into a fresh short array.
> 409:      * @return a fresh short array copy of this memory segment.
> 410:      * @throws UnsupportedOperationException if this segment's contents cannot be copied into a {@link short[]} instance,

I realize this is commenting on prior API artifacts... `UnsupportedOperationException` may be misleading since the behaviour is dependent on the segment's state. `IllegalStateException` is overloaded but that seems like the right one to use.

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

> 470:      * and ends relative to the buffer's limit (exclusive).
> 471:      * <p>
> 472:      * If the buffer is read-only (see {@link ByteBuffer#isReadOnly()}), the resulting segment will also be read-only

One JavaDoc technique is to link the relevant text directly, such as `{@link ByteBuffer#isReadOnly() read-only}` in the "e.g." or "see" cases, making for more concise specification. The `@see` annotation is also useful in this context.

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

> 592:      * }</pre></blockquote>
> 593:      *
> 594:      * @implNote The block of off-heap memory associated with the returned native memory segment is initialized to zero.

Is that really an implementation detail? i.e. another implementation could choose not to zero?
This seems something that should be specified and required.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 111:

> 109:      * @param bytesSize the desired size.
> 110:      * @return a new native memory segment with given base address and size.
> 111:      * @throws IllegalArgumentException if {@code bytesSize <= 0}.

`<= 0` or `< 0` ? The latter is specified by `MemorySegment.allocateNative`, but i think the implementation behaves like the former?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 81:

> 79:  * owner thread will result in a runtime failure.
> 80:  * <p>
> 81:  * Shared resource scopes (see {@link #ofShared()}), support strong thread-confinement guarantees. A shared resource scope

", have no thread-confinement guarantees"?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SegmentAllocator.java line 464:

> 462: 
> 463:     /**
> 464:      * Returns a native allocator which responds to allocation requests by recycling a single segment; that is,

"Returns a segment allocator... " ?

AFAICT this is the only one that is not a native allocator. Perhaps its name need to reflect something more specific.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/package-info.java line 68:

> 66:  * and in a timely fashion. For this reason, there might be cases where waiting for the garbage collector to determine that a segment
> 67:  * is <a href="../../../java/lang/ref/package.html#reachability">unreachable</a> is not optimal.
> 68:  * Clients that operate under these assumptions might want to be able to programmatically release the memory associated

Suggestion:

 * Clients that operate under these assumptions might want to programmatically release the memory associated

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/package-info.java line 80:

> 78:  * }</pre>
> 79:  *
> 80:  * This example is almost identical to the one shown above; this time we first create a so called <em>resource scope</em>,

Suggestion:

 * This example is almost identical to the prior one; this time we first create a so called <em>resource scope</em>,

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java line 30:

> 28:     MemorySegment newSegment(long size, long align) {
> 29:         MemorySegment segment = allocator.allocate(size, align);
> 30:         return segment;

Suggestion:

        return allocator.allocate(size, align);
Thereby no shadowing of the segment field.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java line 9:

> 7: import java.util.OptionalLong;
> 8: 
> 9: public class ArenaAllocator implements SegmentAllocator {

The implementations are not thread safe. Do we need to adjust the specification on SegmentAllocator accordingly?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ConfinedScope.java line 38:

> 36:  * can be performed in plain mode.
> 37:  */
> 38: class ConfinedScope extends MemoryScope {

Suggestion:

final class ConfinedScope extends MemoryScope {

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ConfinedScope.java line 89:

> 87:      * A confined resource list; no races are possible here.
> 88:      */
> 89:     static class ConfinedResourceList extends ResourceList {

Suggestion:

    static final class ConfinedResourceList extends ResourceList {

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ConfinedScope.java line 115:

> 113:      * A confined resource scope handle; no races are possible here.
> 114:      */
> 115:     class ConfinedHandle implements Handle {

Suggestion:

    final class ConfinedHandle implements Handle {

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

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

More information about the panama-dev mailing list