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

[Patricio Chilano Mateo]

On Mon, 28 Mar 2022 04:38:33 GMT, David Holmes <dholmes at openjdk.org> wrote:

> Hi Patricio,
> 
> This looks really good, though I have a few comments and queries below.
> 
Thanks for the review David! See comments below.

Patricio

> 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?

Yes, almost always they are call by the current thread to check its own queue. The exceptions would be has_operation() which is also called by the handshaker in HandshakeState::try_process(), and now has_async_exception_operation(bool) which is also called by the sender of the async exception to verify there is no ThreadDeath already pending in the target's queue.

> 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/

Fixed.

> 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 ??

I looked at the suspend handshakes (ThreadSelfSuspensionHandshake, SuspendThreadHandshake) and followed that naming convention. But now doing a grep I see that most handshakes have Closure instead at the end, some have HandshakeClosure, and some just have handshake at the beginning(HandshakeForDeflation, HandshakeForPD) . Not sure now which one we should use. It seems it should be either Closure or HandshakeClosure at the end.
Regarding InstallAsyncExceptionHandshake, we need it to execute the first handshake that installs the actual asynchronous handshake. The closure of that asynchronous handshake is stored in InstallAsyncExceptionHandshake::_aeh.

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

Fixed.

> 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?

I added that overload for Panama, in case they decide to use asynchronous handshakes to solve their cleanup of mapped memory issues. Same thing with virtual method should_throw() in AsyncExceptionHandshake, since they wanted to do some check first in the handshake to decide if they should throw. But I could remove them.

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

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