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

[Daniel D.Daugherty]

On Thu, 24 Mar 2022 20:00:59 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> 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...
>
> Maybe non_self_executable_filter? Or non_handshakee_executable_filter?

`non_self_executable_filter`  sounds good to me.

So does this filter mean: Return things that don't have to be executed by the handshakee.

>> 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.
>
> Nice table! Yes, that's all correct.

Thanks for confirming.

>> 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`.
>
> I checked the method and the name of that boolean parameter is just 'val', so not really meaningful. Should I write 'interrupted' instead? That is the name of the field we are changing in class Thread.

I'm not sure where my head was at. That's just the "new_value" for the set operation
so you don't need a comment there. Sorry for the noise.

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

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