[foreign-memaccess+abi] RFR: API refresh - part two [v3]

[Paul Sandoz]

On Wed, 6 Oct 2021 13:52:49 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> After playing a bit more with the new API changes, I realized there were issues with some of the solutions proposed in [1].
>> I will add more details in a separate comment, to keep the header of this PR compact.
>> 
>> [1] - https://git.openjdk.java.net/panama-foreign/pull/576
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix code snippet in SegmentAllocator::nativeAllocator

We went round and round trying to lump SA and RS together but it was a bit like magnets repelling each other. But, i think we have come away with a better appreciation of the patterns where SA should be used. (And keeping with the separation will have a good effect on how we can better make scopes be dependent on other scopes.)

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

> 149: MemorySegment segment = null;
> 150: try (ResourceScope scope = ResourceScope.newConfinedScope()) {
> 151:     segment = MemorySegment.allocateNative(8);

Suggestion:

    segment = MemorySegment.allocateNative(8, scope);

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

> 74:  *
> 75:  * An important implicit resource scope is the so called {@linkplain #globalScope() global scope}; the global scope is
> 76:  * a resource scope that cannot be closed, either explicitly or implicitly. As a results, the global scope will never

Suggestion:

 * a resource scope that cannot be closed, either explicitly or implicitly. As a result, the global scope will never

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

> 202: 
> 203:     /**
> 204:      * Create a new confined scope.

Suggestion:

     * Creates a new confined scope.

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

> 210: 
> 211:     /**
> 212:      * Create a new confined scope, managed by the provided cleaner instance.

Suggestion:

     * Creates a new confined scope, managed by the provided cleaner instance.

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

> 220: 
> 221:     /**
> 222:      * Create a new shared scope.

Suggestion:

     * Creates a new shared scope.

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

> 228: 
> 229:     /**
> 230:      * Create a new shared scope, managed by the provided cleaner instance.

Suggestion:

     * Creates a new shared scope, managed by the provided cleaner instance.

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

> 361:      * @param scope the scope associated with the segments returned by the arena-based allocator.
> 362:      * @return a new unbounded arena-based allocator
> 363:      * @throws IllegalArgumentException if {@code blockSize <= 0}, if {@code arenaSize <= 0} or if {@code arenaSize < blockSize}.

Suggestion:

     * @throws IllegalArgumentException if {@code arenaSize <= 0}.

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

> 392:      * <p>
> 393:      * The returned allocator might throw an {@link OutOfMemoryError} if the total memory allocated with this allocator
> 394:      * exceeds the arena size, or the system capacity. Furthermore, the returned allocator is not thread safe, and all

The "... returned allocator is not thread safe ..." contradicts the prior paragraph stating its thread safe for a shared allocator, which is no longer the case.

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

> 130:     /**
> 131:      * Copies this variable argument list at its current position into a new variable argument list associated
> 132:      * with the same scope as this variable argument list. using the segment provided allocator. Copying is useful to

What is the "segment provided allocator"? Do you mean `MemorySegment.allocateNative`?

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

> 117: 
> 118:       try (var scope = ResourceScope.newConfinedScope()) {
> 119:          var cString = MemorySegment.allocateNative(5 + 1);

Suggestion:

         var cString = MemorySegment.allocateNative(5 + 1, scope);

test/jdk/java/foreign/TestByteBuffer.java line 500:

> 498:         f.createNewFile();
> 499:         f.deleteOnExit();
> 500:         MemorySegment.mapFile(f.toPath(), 0L, -1, FileChannel.MapMode.READ_WRITE, ResourceScope.newSharedScope());

Shared scope or implicit scope?

test/micro/org/openjdk/bench/jdk/incubator/foreign/CallOverheadHelper.java line 85:

> 83: 
> 84:     static final MemorySegment sharedPoint = MemorySegment.allocateNative(POINT_LAYOUT, ResourceScope.newImplicitScope());
> 85:     static final MemorySegment confinedPoint = MemorySegment.allocateNative(POINT_LAYOUT, ResourceScope.newImplicitScope());

Uses implicit rather than confined scope, so now it has the same properties as `sharedPoint`.

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

Marked as reviewed by psandoz (Committer).

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