RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v2]

[Paul Sandoz]

On Tue, 12 Oct 2021 20:51:02 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR contains the API and implementation changes for JEP-419 [1]. A more detailed description of such changes, to avoid repetitions during the review process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix TestLinkToNativeRBP test

Like with previous reviews of code for Panama JEPs there are not many comments on this PR, since prior reviews occurred for PRs in the panama-foreign repo.

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

> 75:  * Furthermore, if the function descriptor's return layout is a group layout, the resulting downcall method handle accepts
> 76:  * an extra parameter of type {@link SegmentAllocator}, which is used by the linker runtime to allocate the
> 77:  * memory region associated with the struct returned by  the downcall method handle.

Suggestion:

 * memory region associated with the struct returned by the downcall method handle.

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

> 86:  * in which the specialized signature of a given variable arity callsite is described in full. Alternatively,
> 87:  * if the foreign library allows it, clients might also be able to interact with variable arity methods
> 88:  * using by passing a trailing parameter of type {@link VaList}.

Suggestion:

 * by passing a trailing parameter of type {@link VaList}.

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

> 143:      * Lookup a symbol in the standard libraries associated with this linker.
> 144:      * The set of symbols available for lookup is unspecified, as it depends on the platform and on the operating system.
> 145:      * @return a linker-specific library lookup which is suitable to find symbols in the standard libraries associated with this linker.

Suggestion:

     * @return a symbol in the standard libraries associated with this linker.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java line 93:

> 91:         Objects.requireNonNull(argLayouts);
> 92:         Arrays.stream(argLayouts).forEach(Objects::requireNonNull);
> 93:         return new FunctionDescriptor(null, argLayouts);

We need to clone `argLayouts`, otherwise the user can modify the contents. Internally, using `List.of` is useful, since it does the cloning and null checks, and that is the type that is returned. Also `argumentLayouts` uses `Arrays.asList` which supports modification of the contents.

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 "the returned allocator is not thread safe" contradicts the prior sentence "An allocator associated with a <em>shared</em> resource scope is thread-safe and allocation requests may be performed concurrently".

Perhaps just say:
"
<p>
The returned allocator is not thread safe if the given scope is a shared scope. Concurrent allocation needs to be guarded with synchronization primitives. 
"

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

> 258: 
> 259:     @Override
> 260:     public final MemorySegment asOverlappingSlice(MemorySegment other) {

Please ignore these comments if you wish.

Adding `max() // exclusive` to complement `min()` might be useful.

In these cases i find it easier to sort the segments e.g. `var a = this; var b = other; if (a.min() > b.min()) { // swap a and b }` then the subsequent logic tends to get simpler e.g. overlap test is `if (b.min() < a.max())`, `segmentOffset` is always +ve.

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

> 98:             do {
> 99:                 value = (int)ASYNC_RELEASE_COUNT.getVolatile(this);
> 100:             } while (!ASYNC_RELEASE_COUNT.compareAndSet(this, value, value + 1));

Use `getAndAdd` (and ignore the return value).

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

PR: https://git.openjdk.java.net/jdk/pull/5907