RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v6]

[Jorn Vernee]

On Mon, 7 Nov 2022 15:00:02 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR contains the API and implementation changes for JEP-434 [1]. A more detailed description of such changes, to avoid repetitions during the review process, is included as a separate comment.
>> 
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make memory session a pure lifetime abstraction

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 157:

> 155:     public long mismatch(MemorySegment other) {
> 156:         Objects.requireNonNull(other);
> 157:         return MemorySegment.mismatch(this, 0, byteSize(), other, 0, other.byteSize());

Bit strange to see this calling back up to a method in the interface. Maybe this should just be a `default` method in `MemorySegment`?

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 163:

> 161:      * Mismatch over long lengths.
> 162:      */
> 163:     public static long vectorizedMismatchLargeForBytes(MemorySessionImpl aSession, MemorySessionImpl bSession,

Does this need to be `public`? Only seems to be referenced below.

src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 179:

> 177:     @ForceInline
> 178:     public static MemorySessionImpl toSessionImpl(MemorySession session) {
> 179:         return (MemorySessionImpl)session;

Maybe calls to this method should just be replaced with a cast.

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64VaList.java line 136:

> 134:         long ptr = UNSAFE.allocateMemory(LAYOUT.byteSize());
> 135:         MemorySegment ms = MemorySegment.ofAddress(ptr, LAYOUT.byteSize(),
> 136:                 MemorySession.implicit(), () -> UNSAFE.freeMemory(ptr));

pre-existing, but it seems like this could just use `MemorySegment.allocateNative(LAYOUT, MemorySession.implicit())`?
Suggestion:

        MemorySegment base = MemorySegment.allocateNative(LAYOUT, MemorySession.implicit());


(and remove the dependency on `Unsafe` altogether)

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64VaList.java line 142:

> 140:         VH_gr_offs.set(ms, 0);
> 141:         VH_vr_offs.set(ms, 0);
> 142:         return ms;

I suggest doing 
Suggestion:

        return ms.asSlice(0, 0);


To create an opaque segment, just like the `segment()` accessor does. Or maybe update the implementation of `SharedUtils.emptyVaList` to do this.

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/linux/LinuxAArch64VaList.java line 408:

> 406:     @Override
> 407:     public MemorySegment segment() {
> 408:         return segment.asSlice(0, 0);

A comment about what is happening here would be nice. (making sure the returned segment is opaque?)

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/macos/MacOsAArch64VaList.java line 176:

> 174:     @Override
> 175:     public MemorySegment segment() {
> 176:         return segment.asSlice(0, 0);

Same here.

src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVVaList.java line 145:

> 143:         long ptr = U.allocateMemory(LAYOUT.byteSize());
> 144:         MemorySegment base = MemorySegment.ofAddress(ptr, LAYOUT.byteSize(),
> 145:                 MemorySession.implicit(), () -> U.freeMemory(ptr));

Same here: `MemorySegment base = MemorySegment.allocateNative(LAYOUT, MemorySession.implicit());`

src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVVaList.java line 150:

> 148:         VH_overflow_arg_area.set(base, MemorySegment.NULL);
> 149:         VH_reg_save_area.set(base, MemorySegment.NULL);
> 150:         return base;

Suggestion:

        return base.asSlice(0, 0);

test/jdk/java/foreign/normalize/TestNormalize.java line 203:

> 201:     public static Object[][] bools() {
> 202:         return new Object[][]{
> 203:             { 0b01,          true  }, // zero least significant bit, but non-zero first byte

According to the comment this should actually be:
Suggestion:

            { 0b10,          true  }, // zero least significant bit, but non-zero first byte


Looks like I wrote this by mistake :(

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

PR: https://git.openjdk.org/jdk/pull/10872