[foreign-preview] RFR: 8281855: Rename ResourceScope to MemorySession [v6]

[Jorn Vernee]

On Mon, 21 Feb 2022 12:10:49 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch implements some of the changes described in [1] and [2]. More specifically, the `ResourceScope` abstraction is renamed into `MemorySession`, and the session accessors in `MemorySegment`, `NativeSymbol` and `VaList` are tweaked to return *non-closeable* session views. To counteract this change, `MemorySession` now supports `equals` and `hashCode`, so that sessions views can be compared to each other in ways that are not identity-sensitive.
>> 
>> [1] - https://mail.openjdk.java.net/pipermail/panama-dev/2022-February/016152.html
>> [2] - https://mail.openjdk.java.net/pipermail/panama-dev/2022-February/016254.html
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix spurious reference to ResourceScopeImpl after merge

src/java.base/share/classes/java/lang/foreign/MemorySession.java line 147:

> 145:      * a thread other than the thread {@linkplain #ownerThread() owning} this memory session.
> 146:      */
> 147:     void addCloseAction(Runnable runnable);

A note here, or on `openImplicit`, saying that a close action should not reference the session itself would be good I think. Suggestion inline (feel free to change).
Suggestion:

    /**
     * Add a custom cleanup action which will be executed when the memory session is closed.
     * The order in which custom cleanup actions are invoked once the memory session is closed is unspecified.
     * <p>
     * Users should take care so that the {@code Runnable} instance provided as a close action does not reference this
     * memory sessison. This will create a reference cycle that will keep the session alive indefinitely if it is expected to be
     * cleanup up by a cleaner.
     * @param runnable the custom cleanup action to be associated with this memory session.
     * @throws IllegalStateException if this memory session is not {@linkplain #isAlive() alive}, or if access occurs from
     * a thread other than the thread {@linkplain #ownerThread() owning} this memory session.
     */
    void addCloseAction(Runnable runnable);

src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 64:

> 62: import java.util.Objects;
> 63: import java.util.function.Consumer;
> 64: import java.util.function.UnaryOperator;

Spurious imports?

src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 499:

> 497:         @Override
> 498:         public MemorySession session() {
> 499:             return new MemorySessionImpl.NonCloseableView(sessionImpl());

Can this just return `MemorySession.global()`?

src/java.base/share/classes/jdk/internal/foreign/abi/UpcallStubs.java line 59:

> 57:             }
> 58:         });
> 59:         return new NativeSymbolImpl("upcall:" + Long.toHexString(entry), MemoryAddress.ofLong(entry), ((MemorySessionImpl)session));

If I understand correctly, this doesn't use `Scoped.toSessionImpl` since the session doesn't need to be closed in this case?

test/jdk/java/foreign/SafeFunctionAccessTest.java line 192:

> 190:     static void checkSession(MemorySession session) {
> 191:         try {
> 192:             ((MemorySession)session).close();

Redundant cast?

test/jdk/java/foreign/TestRestricted.java line 69:

> 67:         var mh = MethodHandles.lookup().findStatic(MemorySegment.class, "ofAddress",
> 68:             MethodType.methodType(MemorySegment.class, MemoryAddress.class, long.class, MemorySession.class));
> 69:         var seg = (MemorySegment)mh.invoke(MemoryAddress.NULL, 4000L, MemorySession.global());

Why is this relaxed to invoke?

test/jdk/java/foreign/valist/VaListTest.java line 132:

> 130:             = actions -> MacOsAArch64Linker.newVaList(actions, MemorySession.openConfined());
> 131:     private static final Function<Consumer<VaList.Builder>, VaList> platformVaListFactory
> 132:             = (builder) -> VaList.make(builder, MemorySession.openConfined());

Now that the tests no longer close the scope, maybe these scopes should be implicit scopes? (at least then we still test the eventual closure of the VaList).

test/micro/org/openjdk/bench/java/lang/foreign/BulkMismatchAcquire.java line 122:

> 120:         if (session instanceof MemorySession) {
> 121:             ((MemorySession) session).close();
> 122:         }

Seems like this check is redundant? Or maybe this should check for isCloseable

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

PR: https://git.openjdk.java.net/panama-foreign/pull/641