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

[Peter Levart]

Hi Maurizio, comments inline...

On 5/15/20 11:44 AM, Maurizio Cimadamore wrote:
> 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


Ok, moving it as 1st parameter.


>
> 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?


If there is a data race (publishing reference to scope), then I think 
JMM may allow this write to end up in final memory location later than 
for example another write that puts true to this field. So while C2 may 
optimize this write into no-op, it may still theoretically cause 
trouble. This is only theory of what is allowed by JMM of course. All 
fields of an object already get their 1st implicit initializing write 
which is different as it is guaranteed to happen before any read of that 
field. An explicit write is not given such guarantee (unless the field 
is final).

While publishing scope is never performed via data race in our case 
(scope is assigned to final field in MemorySegment), it is a good 
property of a class to allow such usage (like String for example) in 
particular if such class is security or stability sensitive and/or it 
may be used elsewhere hypothetically. So initializing plain fields to 
their default values is never a good thing and is, IMHO just a relic 
from C, where they did not get the implicit initialization. I think Java 
is old enough that everybody knows this difference even without hints in 
comments. So maybe just remove the comment?


>
> 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.


Better. Will change.


>
> 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


Will change.


>
> 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


If I make it private, it is not accessible in subclasses although they 
are nestmates unless I do something like that:


((MemoryScope)this).checkAliveConfined();


Do you prefer such call gymnastics? If this class gets used elsewhere 
then maybe it will be required...


>
> 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.


So above, the check is made against the parameter "owner" not the field 
"owner". This means that we are preventing another usecase where one 
would want to "transfer" ownership to nobody - when one would start with 
confined segment and then wanted to obtain an unconfined segment from it.

This is already prevented in the MemorySegment too:

     public MemorySegment withOwnerThread(Thread newOwner) {
         Objects.requireNonNull(newOwner);

So maybe just rename the parameter to newOwner ?


>
> 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).


I think the sharing can be performed in a more readable way. Will show 
in an update...


Regards, Peter


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