{{ message }}

Closed

# Move "owner" field and thread-confinement checks to MemoryScope#167

## Conversation

### plevart commented May 15, 2020 • edited by openjdk bot

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.

### Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed

### Reviewers

$git fetch https://git.openjdk.java.net/panama-foreign pull/167/head:pull/167 $ git checkout pull/167

 Move "owner" field and thread-confinement checks from MemorySegment t… 
 0107a42 
…o MemoryScope

### bridgekeeper bot commented May 15, 2020

 👋 Welcome back plevart! A progress list of the required criteria for merging this PR into foreign-memaccess will be added to the body of your pull request.
bot added the label May 15, 2020

### Webrevs

reviewed

 Overall, I like this and I agree that moving ownership in scope makes the code easier to follow. I've added some specific comments.
 * the returned scope is not thread-confined and not checked. * @return a root MemoryScope */ static MemoryScope createUnchecked(Object ref, Runnable cleanupAction, Thread owner) {

#### mcimadamore May 15, 2020 Collaborator

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

 } boolean closed = false; private final Thread owner; boolean closed; // = false

#### mcimadamore May 15, 2020 Collaborator

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?

#### mcimadamore May 15, 2020 Collaborator

So maybe just rename the parameter to newOwner ?

 * If this is a root scope, new root scope is returned, this root scope is closed but * eventual cleanup action is not executed yet - it is inherited by duped scope.
Comment on lines 138 to 139

#### mcimadamore May 15, 2020 Collaborator

Suggested change
 * If this is a root scope, new root scope is returned, this root scope is closed but * eventual cleanup action is not executed yet - it is inherited by duped scope. * 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.
 * If this is a child scope, new child scope is returned. * This method may only be called in "owner" thread of this scope unless the
Comment on lines 140 to 141

#### mcimadamore May 15, 2020 Collaborator

Suggested change
 * If this is a child scope, new child scope is returned. * This method may only be called in "owner" thread of this scope unless the * 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
 * * @throws IllegalStateException if this scope is already closed */ @ForceInline final void checkAliveConfined() {

#### mcimadamore May 15, 2020 Collaborator

Consider making this private

 MemoryScope dup() { // always called in owner thread return closeOrDup(false); MemoryScope dup(Thread owner) { Objects.requireNonNull(owner, "owner");

#### mcimadamore May 15, 2020 Collaborator

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.

 } private MemoryScope closeOrDup(boolean close) { private MemoryScope closeOrDup(Thread newOwner) {

#### mcimadamore May 15, 2020 Collaborator

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

### mlbridge bot commented May 15, 2020

 Mailing list message from Peter Levart on panama-dev: 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 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
 Improvements to MemoryScope code considering Maurizio's comments. 
 3845bcf 

### mcimadamore commented May 15, 2020

 So maybe just remove the comment? No, leave it there - I'm fine with the explanation.

### mcimadamore commented May 15, 2020

 ((MemoryScope)this).checkAliveConfined(); Do you prefer such call gymnastics? If this class gets used elsewhere then maybe it will be required... Ugh - I missed subclass usages. Perhaps a private static routine in the toplevel class. But it's also ok if you leave it like that.
approved these changes

 Looks good. Thanks.
 } @Override void close() { closeOrDup(null); justClose();

#### mcimadamore May 15, 2020 Collaborator

This is exactly what I had in mind - I think the code is cleaner. Perhaps rename justClose to doClose (and make doClose private) - but that's minor - I'll leave that up to you.

#### plevart May 15, 2020 Author Contributor

I think justClose() tells more that doClose() and private it is.

### openjdk bot commented May 15, 2020 • edited

 @plevart This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be: Move "owner" field and thread-confinement checks to MemoryScope Reviewed-by: mcimadamore  If you would like to add a summary, use the /summary command. To credit additional contributors, use the /contributor command. To add additional solved issues, use the /issue command. Since the source branch of this PR was last updated there have been 113 commits pushed to the foreign-memaccess branch: 39ab618: Automatic merge of master into foreign-memaccess 37d5e5f: Automatic merge of jdk:master into master 2349db7: Add MemorySegment::fill 82f2a0e: 8245024: Simplify and eagerly initialize StringConcatFactory b76a215: 8245046: SetupTarget incorrect for hotspot-ide-project 4c54fa2: 8209774: Refactor shell test javax/xml/jaxp/common/8035437/run.sh to java b883bad: 8244961: MethodHandles::privateLookupIn throws NPE when called during initPhase2 cab61f1: 8243012: Fix issues in j.l.i package info 8da07d1: 8242524: Use different default CDS archives depending on UseCompressOops 71cc95e: 8243947: [TESTBUG] hotspot/jtreg:hotspot_appcds_dynamic fails when the JDK doesn't have default CDS archive ... and 103 more: https://git.openjdk.java.net/panama-foreign/compare/667f7f0288bc79736c50cdfc2e10945168f1a124...foreign-memaccess As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge foreign-memaccess into your branch, and then specify the current head hash when integrating, like this: /integrate 39ab61868cc2c968033d0b3d1c1755717b833f42. As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
bot added the label May 15, 2020
 Make private API really private in MemoryScope 
 ad3b5e5 

### plevart commented May 15, 2020

 I hid checkAliveConfined (by making it private static) and also the closed field which has the same poblem of being access from subclass if it is private. But there's no such problem if it is accessed via VarHandle. Would that be OK?

### plevart commented May 15, 2020

 I ran the tests and the TestSegments.testOpOutsideConfinement(close) fails with this change. The fix follows.
 Fix confined root MemoryScope.close() - confinement check is required 
 9fdd5a0 

### mcimadamore commented May 15, 2020

 I ran the tests and the TestSegments.testOpOutsideConfinement(close) fails with this change. The fix follows. In the latest iteration I don't see any change that could possibly affect the test (compared to the previous iteration I mean). Are you also working on a followup fix for the test?
reviewed
 if (owner != null) { if (owner != Thread.currentThread()) { throw new IllegalStateException("Attempted access outside owning thread"); } checkAliveConfined(this); }
Comment on lines 187 to 192

#### JornVernee May 15, 2020 • edited Member

This changes the behavior we had before in AbstractMemorySegmentImpl::checkValidState. We should still do the liveness check even if there is no owner.

Suggested change
 if (owner != null) { if (owner != Thread.currentThread()) { throw new IllegalStateException("Attempted access outside owning thread"); } checkAliveConfined(this); } if (owner != null && owner != Thread.currentThread()) { throw new IllegalStateException("Attempted access outside owning thread"); } checkAliveConfined(this);

#### mcimadamore May 15, 2020 Collaborator

This changes the behavior we had before in AbstractMemorySegmentImpl::checkValidState. We should still do the liveness check even if there is no owner.

True - I suspect Peter would argue that such check doesn't make much sense in a racy context - but still, there's a change in behavior here.

#### JornVernee May 15, 2020 • edited Member

IMO It's not necessarily racy, it's just that the API doesn't guarantee that there is no race, that's up to the user to do. e.g. by making sure that the thread doing the access can see an up to date liveness flag (by using other synchronization).

#### plevart May 15, 2020 Author Contributor

Ok, then another change follows...

approved these changes

 Looks good - thanks for spotting the test issue.
 Perform liveness check for unconfined MemoryScope too even if it may … 
 66a821d 
…be racy without outside synchronization

### plevart commented May 15, 2020

 Tests still pass after this change.
approved these changes

 LGTM

### plevart commented May 15, 2020

 /integrate
bot added the label May 15, 2020

### mcimadamore commented May 15, 2020

bot closed this May 15, 2020
bot added and removed labels May 15, 2020
bot removed the label May 15, 2020