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

Tue Nov 2 17:32:24 UTC 2021

On Mon, 1 Nov 2021 12:05:32 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 with a new target base due to a merge or a rebase. The pull request now contains 17 commits:
> 
>  - Add cache for memory address var handles
>  - Merge branch 'master' into JEP-419
>  - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
>  - Merge branch 'master' into JEP-419
>  - Fix copyright header in TestArrayCopy
>  - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
>  - * use `invokeWithArguments` to simplify new test
>  - Add test for liveness check with high-aririty downcalls
>    (make sure that if an exception occurs in a downcall because of liveness,
>    ref count of other resources are left intact).
>  - * Fix javadoc issue in VaList
>    * Fix bug in concurrent logic for shared scope acquire
>  - Address review comments
>  - ... and 7 more: https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1586:

> 1584:             public void ensureCustomized(MethodHandle mh) {
> 1585:                 mh.customize();
> 1586:             }

This is no longer needed, but it probably got picked up in the merge.

src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java line 144:

> 142:      * @param mh the method handle
> 143:      */
> 144:     void ensureCustomized(MethodHandle mh);

Same here, no longer needed. (it was used by now removed upcall handler code. See https://github.com/openjdk/panama-foreign/pull/553)

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

> 105:      *
> 106:      * @param offset offset in bytes (relative to this address). The final address of this read operation can be expressed as {@code toRowLongValue() + offset}.
> 107:      * @return a Java UTF-8 string containing all the bytes read from the given starting address ({@code toRowLongValue() + offset})

(see also comment on MemorySegment.getUtf8String)
Suggestion:

     * @return a Java string constructed from the bytes read from the given starting address ({@code toRowLongValue() + offset})

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

> 385: 
> 386:     /**
> 387:      * Performs an element-wise bulk copy from given source segment to this segment. More specifically, the bytes at

Suggestion:

     * Performs a byte-wise bulk copy from given source segment to this segment. More specifically, the bytes at

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

> 398:      * a multiple of the source element layout size, if the source segment is incompatible with the alignment constraints
> 399:      * in the source element layout, or if this segment is incompatible with the alignment constraints
> 400:      * in the destination element layout.

This speaks about element layouts, but I don't see any element layouts in the method implementation.

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

> 631:      * java.nio.charset.CharsetDecoder} class should be used when more control
> 632:      * over the decoding process is required.
> 633:      * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative()} segment,

Suggestion:

     * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative() native} segment,

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

> 634:      *               the final address of this read operation can be expressed as {@code address().toRowLongValue() + offset}.
> 635:      * @return a Java UTF-8 string containing all the bytes read from the given starting address up to (but not including)
> 636:      * the first {@code '\0'} terminator character (assuming one is found).

The phrase "a Java UTF-8 string" sounds strange to me, as Java Strings are not encoded in UTF-8. The string that is read is UTF-8 encoded, but then it is converted from UTF-8 to Java internal String encoding (UTF-16 or Latin1).

I'd suggest just dropping the 'UTF-8', and changing 'containing all' to 'constructed from'.
Suggestion:

     * @return a Java string constructed from the bytes read from the given starting address up to (but not including)
     * the first {@code '\0'} terminator character (assuming one is found).

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

> 650:      * java.nio.charset.CharsetDecoder} class should be used when more control
> 651:      * over the decoding process is required.
> 652:      * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative()} segment,

Suggestion:

     * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative() native} segment,

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

> 760: 
> 761:     /**
> 762:      * Creates a new native memory segment with given size and resource scope, and whose base address is this address.

Suggestion:

     * Creates a new native memory segment with given size and resource scope, and whose base address is the given address.

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

> 767:      * provided resource scope.
> 768:      * <p>
> 769:      * Clients should ensure that the address and bounds refers to a valid region of memory that is accessible for reading and,

Suggestion:

     * Clients should ensure that the address and bounds refer to a valid region of memory that is accessible for reading and,

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

> 1033:      *
> 1034:      * @param layout the layout of the memory region to be read.
> 1035:      * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative()} segment,

Suggestion:

     * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative() native} segment,

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

> 1547:      * @param index index (relative to this segment). For instance, if this segment is a {@link #isNative()} segment,
> 1548:      *               the final address of this write operation can be expressed as {@code address().toRowLongValue() + (index * layout.byteSize())}.
> 1549:      * @param value the byte value to be written.

Suggestion:

     * @param value the address value to be written.

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

> 1561:      * Copies a number of elements from a source segment to a destination array,
> 1562:      * starting at a given segment offset (expressed in bytes), and a given array index, using the given source element layout.
> 1563:      * Supported array types are {@code byte[]}, {@code char[]},{@code short[]},{@code int[]},{@code float[]},{@code long[]} and {@code double[]}.

Suggestion:

     * Supported array types are {@code byte[]}, {@code char[]}, {@code short[]}, {@code int[]}, {@code float[]}, {@code long[]} and {@code double[]}.

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

> 1602:      * Copies a number of elements from a source array to a destination segment,
> 1603:      * starting at a given array index, and a given segment offset (expressed in bytes), using the given destination element layout.
> 1604:      * Supported array types are {@code byte[]}, {@code char[]},{@code short[]},{@code int[]},{@code float[]},{@code long[]} and {@code double[]}.

Suggestion:

     * Supported array types are {@code byte[]}, {@code char[]}, {@code short[]}, {@code int[]}, {@code float[]}, {@code long[]} and {@code double[]}.

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

> 206:      */
> 207:     static ResourceScope newConfinedScope() {
> 208:         return ResourceScopeImpl.createConfined( Thread.currentThread(), null);

Suggestion:

        return ResourceScopeImpl.createConfined(Thread.currentThread(), null);

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

I think ". using the segment provided allocator" can be removed. Seems like a leftover from when we had an overload that took an allocator.
Suggestion:

     * with the same scope as this variable argument list. Copying is useful to

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

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