[foreign-memaccess] RFR: Move "owner" field and thread-confinement checks to MemoryScope

[Maurizio Cimadamore]

On Fri, 15 May 2020 08:56:14 GMT, Peter Levart <plevart at openjdk.org> wrote:

> Now MemoryScope is simplified, I re-based this change and am opening this PR on top. Currently MemorySegment is
> encapsulating thread-confinement logic and state (owner field) while MemoryScope is encapsulating temporal-confinement
> logic and state. But the interplay between the two must be very carefully caried out (for example, close() or dup() on
> child scopes may only be called in owner thread). By moving the thread-confinement logic and state to MemoryScope, I
> think we get better encapsulation as all MemoryScope methods become "safe" - some can still be called in owner thread
> only, but failing to do so throws IllegalSateException instead of exhibiting undefined behavior.

Overall, I like this and I agree that moving ownership in scope makes the code easier to follow. I've added some
specific comments.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 90:

> 89:      */
> 90:     static MemoryScope createUnchecked(Object ref, Runnable cleanupAction, Thread owner) {
> 91:         return new Root(owner, ref, cleanupAction);

For some (merely stylistic) reason, I'd prefer to see `owner` coming first in the parameter list

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 95:

> 94:     private final Thread owner;
> 95:     boolean closed; // = false
> 96:     private static final VarHandle CLOSED;

Is there any real advantage in not having the initialization here? I mean, I get we probably save one putfield - but I
would say that if the code is hot, C2 is probably able to detect that the store is useless? I kind of prefer the code
to be self-evident rather then to use comments, where possible, of course. I've seen in the past cases where seemingly
innocuous redundant initializer subtly changed happens-before order; but this shouldn't be the case here?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 139:

> 138:      * If this is a root scope, new root scope is returned, this root scope is closed but
> 139:      * eventual cleanup action is not executed yet - it is inherited by duped scope.
> 140:      * If this is a child scope, new child scope is returned.

Suggestion:

     * If this is a root scope, a new root scope is returned; this root scope is closed, but
     * without executing the cleanup action, which is instead transferred to the duped scope.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 141:

> 140:      * If this is a child scope, new child scope is returned.
> 141:      * This method may only be called in "owner" thread of this scope unless the
> 142:      * scope is a root scope with no owner thread - i.e. is not checked.

Suggestion:

     * If this is a child scope, a new child scope is returned.
     * This method may only be called in the "owner" thread of this scope unless the

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 203:

> 202:     @ForceInline
> 203:     final void checkAliveConfined() {
> 204:         if (closed) {

Consider making this private

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 250:

> 249:         MemoryScope dup(Thread owner) {
> 250:             Objects.requireNonNull(owner, "owner");
> 251:             return closeOrDup(owner);

Uhm - not sure about this. Let's say that you have an unchecked segment - you start off unconfined - but then, after
some processing, you want to kill the segment and obtained a confined view. Seems a legitimate use case? Although I
agree that, in that case, we'd need at least some sort of atomic handshake on the closed bit, to ensure the update
either fails or succeeds across multiple racy threads.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 259:

> 258:
> 259:         private MemoryScope closeOrDup(Thread newOwner) {
> 260:             // pre-allocate duped scope so we don't get OOME later and be left with this scope closed

I think I don't like this way of sharing the code. Since we're in cleanup mode, I think the code would be more readable
by having two separate methods `close` and `dup` which both uses the same underlying logic for flipping the liveness
bit (using the lock).

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

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