On Thu, 23 Nov 2023 11:14:29 GMT, Erik Ă–sterlund <eosterl...@openjdk.org> wrote:

> The current logic for closing memory in panama today is susceptible to live 
> lock if we have a closing thread that wants to close the memory in a loop 
> that keeps failing, and a bunch of accessing threads that want to perform 
> accesses as long as the memory is alive. They can both create impediments for 
> the other.
> 
> By using asynchronous handshakes to install an exception onto threads that 
> are in @Scoped memory accesses, we can have close always succeed, and the 
> accessing threads bail out. The idea is that we perform a synchronous 
> handshake first to find threads that are in scoped methods. They might 
> however be in the middle of throwing an exception or something wild like 
> there, where an exception can't be delivered. We install an async handshake 
> that will roll us forward to the first place where we can indeed install 
> exceptions, then we reevaluate if we still need to do that, or if we have 
> unwound out from the scoped method. If we are still inside of it, we ensure 
> an exception is installed so we don't continue executing bytecodes that might 
> access the memory that we have freed.
> 
> Tested tier 1-5 as well as running test/jdk/java/foreign/TestHandshake.java 
> hundreds of times, which tests this API pretty well.

src/hotspot/share/prims/scopedMemoryAccess.cpp line 151:

> 149:     ResourceMark rm;
> 150:     if (_session != nullptr && last_frame.is_compiled_frame() && 
> last_frame.can_be_deoptimized()) {
> 151:       CloseScopedMemoryFindOopClosure cl(_session);

Pre-existing, but this value (and class) is unused since we do an unconditional 
deopt. If you feel like it, you could remove the 
`CloseScopedMemoryFindOopClosure`. We can get it back from the git history 
later when that bug is fixed (https://bugs.openjdk.org/browse/JDK-8290892)

src/java.base/share/classes/jdk/internal/foreign/SharedSession.java line 86:

> 84:             throw alreadyAcquired(prevState);
> 85:         }
> 86:         SCOPED_MEMORY_ACCESS.closeScope(this);

đŸ¥³

src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
 line 87:

> 85: 
> 86:     public void closeScope(MemorySessionImpl session) {
> 87:         closeScope0(session, MemorySessionImpl.ALREADY_CLOSED);

I suggest passing in the `ALREADY_CLOSED` instance as an argument to this 
method instead. Then we can avoid making the field in `MemorySessionImpl` 
public.

test/jdk/java/foreign/TestHandshake.java line 107:

> 105:                     if (!failed.get()) {
> 106:                         // ignore - this means segment was alive, but 
> was closed while we were accessing it
> 107:                         // next isAlive test should fail

If we see the exception, we should be able to test that the scope is not alive 
here as well
Suggestion:

                        // next isAlive test should fail
                        assertFalse(segment.scope().isAlive());

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403266514
PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403255404
PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403257610
PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403330411

Reply via email to