RFR: 8283044: Use asynchronous handshakes to deliver asynchronous exceptions

[Daniel D.Daugherty]

On Wed, 23 Mar 2022 19:45:18 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review the following patch to use asynchronous handshakes to handle asynchronous exceptions. The patch is broken down in 3 commits:
> 
> 1) A small comment/code cleanup that applies to the current code even without this patch. It attempts to improve commentaries on the issues of handling asynchronous exceptions when coming from compiled code.
> 
> 2) Implementation of send_async_exception(). This commit is the core of the implementation. Some things about the implementation:
> 
> - If we try to send a new async exception to a JT that already has a pending ThreadDeath in the handshake queue then we skip installing the async handshake. This resembles the current behavior where we don't overwrite _pending_async_exception if it is set to a ThreadDeath oop already.
> 
> - If we try to send a new exception to a JT that already has a pending async exception different that ThreadDeath in the handshake queue then we do install the async handshake. Since the handshake queue is a FIFO the last delivered exception will prevail on the _pending_exception field. This also resembles the current behavior where if _pending_async_exception is set but is not a ThreadDeath we overwrite it with the new one.
> 
> - Some key methods from the SafepointMechanism and HandshakeState class were modified to receive an extra boolean to decide when to check for async exceptions, since as with suspend requests we don't want to process them on every poll. These changes still keep the current behavior of processing async exceptions only on transitions back to Java where we don't specifically disallow them (of course since we now arm the poll, the condition will be identified much faster but the processing is still done in the same allowed places as today). 
> 
> 3) Implementation of unsafe access error. I added special method HandshakeState::handle_unsafe_access_error() which takes care of creating the exception oop and calling handle_async_exception() to install the exception.
> 
> I added test AsyncExceptionTest.java to stress delivery of async exceptions with compiled methods on the stack. I also added test AsyncExceptionOnMonitorEnter.java to stress delivery of async exceptions while target is trying to acquire a monitor. The test has two modes, one uses Java monitors and the other one JVMTI raw monitors.
> 
> Besides those new added tests I tested the patch in mach5 tiers1-7.
> 
> Thanks,
> Patricio

Thumbs up. Your call on whether to address my comments; I don't think
any of them are must-do.

src/hotspot/share/runtime/handshake.cpp line 452:

> 450: // Filters
> 451: static bool non_self_queue_filter(HandshakeOperation* op) {
> 452:   return !op->is_async();

I'm still having trouble with the name `non_self_queue` and
the logic in that filter: `!op->is_suspend()`. Maybe a comment
would help future readers from wondering...

src/hotspot/share/runtime/handshake.cpp line 467:

> 465: }
> 466: 
> 467: HandshakeOperation* HandshakeState::get_op_for_self(bool allow_suspend, bool check_async_exception) {

I need a truth table to sort out what this function does. Sorry...

suspend  async  async-blocked  result
-------  -----  -------------  ------
false    false  false          peek w/ no_suspend_no_async_exception_filter
false    false  true           peek w/ no_suspend_no_async_exception_filter
false    true   false          not allowed; asserts
false    true   true           not allowed; asserts
true     false  false          peek w/ no_async_exception_filter
true     false  true           peek w/ no_async_exception_filter
true     true   false          peek w/ no filter
true     true   true           peek w/ no_async_exception_filter

So we're not allowed to ask for async exceptions by themselves,
i.e., {false, true, D/C} always asserts. Okay I can see that.

If we're not interested in suspends, then we're not interested in
asyncs either so we peek for any handshakes that are not suspend
and not async. Okay.

If we're interested in suspend and interested in asyncs:
  - if we're blocking asyncs, then we peek for any handshakes that are not async
  - else we peek for any handshake at all

Okay.

If we're interested in suspend and not interested in async,
then we peek for any handshakes that are not async. Okay.

I think this all makes sense to me now.

src/hotspot/share/runtime/handshake.cpp line 766:

> 764:     // the end of the queue and try again in the next attempt.
> 765:     Handshake::execute(new UnsafeAccessErrorHandshake(), _handshakee);
> 766:     log_error(handshake)("JavaThread " INTPTR_FORMAT " skipping unsafe accesss processing due to suspend.", p2i(_handshakee));

Typo: s/accesss/access/

src/hotspot/share/runtime/handshake.cpp line 771:

> 769:   // Release the handshake lock before constructing the oop to
> 770:   // avoid deadlocks since that can block. This will allow the
> 771:   // JavaThread to execute normally as if it would be outside

Typo: s/would be/was/

May want to "reflow" the remainder of the paragraph.

src/hotspot/share/runtime/safepoint.cpp line 979:

> 977:     }
> 978: 
> 979:     // If an exception has been installed we must check the top frame wasn't deoptimized.

Perhaps:
`// If an exception has been installed we must verify that the top frame wasn't deoptimized.`

src/hotspot/share/runtime/thread.cpp line 1628:

> 1626:           frame f = last_frame();
> 1627:           ls.print(" (pc: " INTPTR_FORMAT " sp: " INTPTR_FORMAT " )", p2i(f.pc()), p2i(f.sp()));
> 1628:         }

Nit - indent is off by one space.

src/hotspot/share/runtime/thread.cpp line 1636:

> 1634: void JavaThread::install_async_exception(AsyncExceptionHandshake* aeh) {
> 1635:   // Do not throw asynchronous exceptions against the compiler thread
> 1636:   // (the compiler thread should not be a Java thread -- fix in 1.4.2)

I would be fine if the comment on line 1636 is deleted.

src/hotspot/share/runtime/thread.cpp line 1646:

> 1644:   // wait()/sleep()/park() and return.
> 1645:   if (has_async_exception_condition(true /* ThreadDeath_only */)) {
> 1646:     java_lang_Thread::set_interrupted(threadObj(), true);

Please add a comment for the parameter name for that `true`.

src/hotspot/share/runtime/thread.cpp line 1664:

> 1662: 
> 1663:   // Interrupt thread so it will wake up from a potential wait()/sleep()/park()
> 1664:   java_lang_Thread::set_interrupted(threadObj(), true);

Please add a comment for the parameter name for that true.

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

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7930