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

[Jorn Vernee]

On Mon, 1 Nov 2021 22:36:40 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:
> 
>   Tweak javadoc of loaderLookup

Mostly some minor javadoc comments.

src/java.base/share/classes/java/lang/Module.java line 32:

> 30: import java.lang.annotation.Annotation;
> 31: import java.lang.invoke.MethodHandle;
> 32: import java.lang.invoke.VarHandle;

These imports seem spurious now.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java line 177:

> 175:         }
> 176:         if (carrier.isPrimitive() && Wrapper.forPrimitiveType(carrier).bitWidth() != size &&
> 177:                 carrier != boolean.class && size != 8) {

I find this condition hard to parse, I'd suggest re-writing it as:

if (carrier.isPrimitive()) {
    long expectedSize = carrier == boolean.class ? 8 : Wrapper.forPrimitiveType(carrier).bitWidth();
    if (size != expectedSize) {
        throw ...
    }
}

(Maybe even change the `if` to an `else` and combine it with the above if).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java line 484:

> 482:     public static final class OfAddress extends ValueLayout {
> 483:         OfAddress(ByteOrder order) {
> 484:             super(MemoryAddress.class, order, Unsafe.ADDRESS_SIZE * 8);

I see `Unsafe.ADDRESS_SIZE` used in several places, suggest to maybe add an `ADDRESS_SIZE_BITS` constants somewhere (it's a bit more readable).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java line 42:

> 40:     final long blockSize;
> 41:     final long arenaSize;
> 42:     final ResourceScope scope;

Could these field be made private?

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

> 86:                     if (size > arenaSize) {
> 87:                         throw new OutOfMemoryError();
> 88:                     }

Isn't this already covered by the `finally` block? Also, this seems to be checking the unaltered `size`, which I think should have been already done at the end of the previous `allocate` call right?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java line 122:

> 120:         ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
> 121:         targetImpl.acquire0();
> 122:         addCloseAction(targetImpl::release0);

Maybe this should explicitly check if target is `null` (though the call to `acquire0` would also produce an NPE, the stack trace having Objects::requireNonNull in there would make the error more obvious I think).
Suggestion:

    public void keepAlive(ResourceScope target) {
        Objects.requireNonNull(target);
        if (target == this) {
            throw new IllegalArgumentException("Invalid target scope.");
        }
        ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
        targetImpl.acquire0();
        addCloseAction(targetImpl::release0);

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java line 101:

> 99:         int value;
> 100:         do {
> 101:             value = (int) STATE.getVolatile(jdk.internal.foreign.SharedScope.this);

Doesn't need to be fully qualified I think?
Suggestion:

            value = (int) STATE.getVolatile(this);

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java line 106:

> 104:                 throw new IllegalStateException("Already closed");
> 105:             }
> 106:         } while (!STATE.compareAndSet(jdk.internal.foreign.SharedScope.this, value, value - 1));

Same here
Suggestion:

        } while (!STATE.compareAndSet(this, value, value - 1));

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

Marked as reviewed by jvernee (Reviewer).

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