RFR: 8312522: Implementation of Foreign Function & Memory API [v10]

Tue Sep 5 22:03:49 UTC 2023

On Mon, 4 Sep 2023 15:54:41 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> This patch contains the implementation of the foreign linker & memory API JEP for Java 22. The initial patch is composed of commits brought over directly from the [panama-foreign repo](https://github.com/openjdk/panama-foreign). The main changes found in this patch come from the following PRs:
>> 
>> 1. https://github.com/openjdk/panama-foreign/pull/836 Where previous iterations supported converting Java strings to and from native strings in the UTF-8 encoding, we've extended the supported encoding to all the encodings found in the `java.nio.charset.StandardCharsets` class.
>> 2. https://github.com/openjdk/panama-foreign/pull/838 We dropped the `MemoryLayout::sequenceLayout` factory method which inferred the size of the sequence to be `Long.MAX_VALUE`, as this led to confusion among clients. A client is now required to explicitly specify the sequence size.
>> 3. https://github.com/openjdk/panama-foreign/pull/839 A new API was added: `Linker::canonicalLayouts`, which exposes a map containing the platform-specific mappings of common C type names to memory layouts.
>> 4. https://github.com/openjdk/panama-foreign/pull/840 Memory access varhandles, as well as byte offset and slice handles derived from memory layouts, now feature an additional 'base offset' coordinate that is added to the offset computed by the handle. This allows composing these handles with other offset computation strategies that may not be based on the same memory layout. This addresses use-cases where clients are working with 'dynamic' layouts, whose size might not be known statically, such as variable length arrays, or variable size matrices.
>> 5. https://github.com/openjdk/panama-foreign/pull/841 Remove this now redundant API. Clients can simply use the difference between the base address of two memory segments.
>> 6. https://github.com/openjdk/panama-foreign/pull/845 Disambiguate uses of `SegmentAllocator::allocateArray`, by renaming methods that both allocate + initialize memory segments to `allocateFrom`. (see the original PR for the problematic case)
>> 7. https://github.com/openjdk/panama-foreign/pull/846 Improve the documentation for variadic functions.
>> 8. https://github.com/openjdk/panama-foreign/pull/849 Several test fixes to make sure the `jdk_foreign` tests can pass on 32-bit machines, taking linux-x86 as a test bed.
>> 9. https://github.com/openjdk/panama-foreign/pull/850 Make the linker API required. The `Linker::nativeLinker` method is not longer allowed to throw an `UnsupportedO...
>
> Jorn Vernee has updated the pull request incrementally with five additional commits since the last revision:
> 
>  - 8315096: Allowed access modes in memory segment should depend on layout alignment
>    
>    Reviewed-by: psandoz
>  - Add missing @implSpec to AddressLayout
>    
>    Reviewed-by: pminborg
>  - Fix misc typos in FFM API javadoc
>    
>    Reviewed-by: jvernee
>  - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle (part two)
>    
>    Reviewed-by: pminborg
>  - Clarify javadoc w.r.t. exceptions thrown by a memory access var handle
>    
>    Reviewed-by: jvernee

Recommend you do a sweep through the code for unused imports and fields and any rogue `@since` in the internal classes.

src/java.base/share/classes/java/lang/foreign/AddressLayout.java line 53:

> 51:  *
> 52:  * @implSpec
> 53:  * This class is immutable, thread-safe and <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>.

Strictly speaking it's the implementations, as stated in the `Linker`:

* Implementations of this interface are immutable, thread-safe and <a href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based</a>.

src/java.base/share/classes/java/lang/foreign/Linker.java line 152:

> 150:  * <p>
> 151:  * The following table shows some examples of how C types are modelled in Linux/x64 (all the examples provided
> 152:  * here will assume these platform-dependent mappings):

Up to you, but it might be useful to link to the ABI specifications if the links are considered stable.

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 439:

> 437:      * </ul>
> 438:      * <p>
> 439:      * If the provided layout path {@code P} contains no dereference elements, then the offset of the access operation is

Suggestion:

     * If the provided layout path {@code P} contains no dereference elements, then the offset {@code O} of the access operation is

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 443:

> 441:      *
> 442:      * {@snippet lang = "java":
> 443:      * offset = this.offsetHandle(P).invokeExact(B, I1, I2, ... In);

Suggestion:

     * O = this.offsetHandle(P).invokeExact(B, I1, I2, ... In);

To align with the use of `O` later on.

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 536:

> 534:      * </ul>
> 535:      * <p>
> 536:      * The offset of the returned segment is computed as if by a call to a

Suggestion:

     * The offset {@code O} of the returned segment is computed as if by a call to a

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 154:

> 152:  *     MemoryLayout.sequenceLayout(4, ValueLayout.JAVA_INT).withName("data") // array of 4 elements
> 153:  * );
> 154:  * VarHandle intHandle = segmentLayout.varHandle(MemoryLayout.PathElemenet.groupElement("data"),

Suggestion:

 * VarHandle intHandle = segmentLayout.varHandle(MemoryLayout.PathElement.groupElement("data"),

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 8027:

> 8025:      * @since 19
> 8026:      */
> 8027:     @PreviewFeature(feature=PreviewFeature.Feature.FOREIGN)

Unused import to `PreviewFeature`, and possibly others too.

src/java.base/share/classes/jdk/internal/foreign/StringSupport.java line 45:

> 43:             case DOUBLE_BYTE -> readFast_short(segment, offset, charset);
> 44:             case QUAD_BYTE -> readFast_int(segment, offset, charset);
> 45:             default -> throw new UnsupportedOperationException("Unsupported charset: " + charset);

Is this necessary, since the switch expression should be exhaustive over all the enum values?

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

PR Review: https://git.openjdk.org/jdk/pull/15103#pullrequestreview-1611924844
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316401360
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316402470
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316409959
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316410079
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316414805
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316437803
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316457079
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1316444767