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