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

[Patricio Chilano Mateo]

On Thu, 24 Mar 2022 17:00:28 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

> Thumbs up. Your call on whether to address my comments; I don't think any of them are must-do.
>
Thanks for the review Dan! See comments below.

Patricio

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

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

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

Fixed.

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

Fixed.

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

Fixed.

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

Removed.

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

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

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