RFR: 8283044: Use asynchronous handshakes to deliver asynchronous exceptions [v3]

[David Holmes]

On Fri, 25 Mar 2022 14:19:57 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
>
> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   change handshake filter name

Hi Patricio,

This looks really good, though I have a few comments and queries below.

Thanks,
David

src/hotspot/share/runtime/handshake.hpp line 134:

> 132:   bool has_operation() { return !_queue.is_empty(); }
> 133:   bool has_operation(bool allow_suspend, bool check_async_exception);
> 134:   bool has_async_exception_operation(bool ThreadDeath_only);

The names of these functions look too much like general queries that might be arbitrarily called, but IIUC these are really only intended to be used by the current thread on its own handshake state - correct?

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

> 1608:       assert(caller_fr.is_compiled_frame(), "what?");
> 1609:       if (caller_fr.is_deoptimized_frame()) {
> 1610:         log_info(exceptions)("deferred async exception at compiled safepoint");

Where is, or should be, this logging in the new code?

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

> 1612:   }
> 1613: 
> 1614:   // Only overwrite an already pending exception if it is not a threadDeath.

Nit: s/threadDeath/ThreadDeath/

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

> 1649:     // We may be at method entry which requires we save the do-not-unlock flag.
> 1650:     UnlockFlagSaver fs(this);
> 1651:     Exceptions::throw_unsafe_access_internal_error(this, __FILE__, __LINE__, "a fault occurred in an unsafe memory access operation");

This seems to be dead code in Exceptions now.

Also the UnlockFlagSaver that was moved into thread.hpp could now be moved back to being a local class in interpreterRuntime.cpp.

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

> 1665: }
> 1666: 
> 1667: class InstallAsyncExceptionHandshake : public HandshakeClosure {

Should this class still have Closure in its name?

I'm unclear why we need this class to wrap an AsyncExceptionHandShake ??

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

> 1669: public:
> 1670:   InstallAsyncExceptionHandshake(AsyncExceptionHandshake* aeh) :
> 1671:   HandshakeClosure("InstallAsyncException"), _aeh(aeh) {}

Nit: needs indentation to show it is continuation of previous line.

src/hotspot/share/runtime/thread.hpp line 819:

> 817:   inline void set_pending_unsafe_access_error();
> 818:   static void send_async_exception(JavaThread* jt, oop java_throwable);
> 819:   static void send_async_exception(JavaThread* jt, AsyncExceptionHandshake* aec);

I'm not seeing where this new method gets called?

src/hotspot/share/runtime/thread.inline.hpp line 129:

> 127: }
> 128: 
> 129: class AsyncExceptionHandshake : public AsyncHandshakeClosure {

Should this class still have closure in its name? I find it hard to understand the difference between a XHandshake and a XClosure.

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

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