RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]

[David Holmes]

On Sat, 16 Oct 2021 16:21:08 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>>> This seems an unrelated change in behaviour ??
>> 
>> Actually this is equivalent code. The baseline code does this:
>> 
>>   ThreadsListHandle tlh;
>>   if (!tlh.includes(this)) {
>>     log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on ThreadsList, no suspension", p2i(this));
>>     return false;
>>   }
>> 
>> 
>> What that code means is: if `this` thread does not appear in the newly created
>> ThreadsListHandle's list, then `this` thread has called `java_suspend()` after
>> `this` thread has exited. That's the only way that `this` thread could be missing
>> from a newly created ThreadsListHandle's list.
>> 
>> All I've done here is get rid of the ThreadsListHandle, verify that the calling
>> context is protecting `this` and switch to an `is_exiting()` call.
>> 
>>> So suspend_thread and resume thread's caller already takes a ThreadsListHandle
>>> so this is unnecessary and never happens.
>> 
>> I presume @coleenp is saying that the `is_exiting()` check is not necessary. 
>> 
>> That might be true, it might not be true. I was trying to create equivalent
>> code without creating a nested ThreadsListHandle here and I think I've
>> done that. I actually think it might be possible for a JVM/TI event handler
>> to fire after the thread has set is_exiting() and for that event handler to
>> call SuspendThread() which could get us to this point.
>
> On rereading all of these comments and the current baseline code, I have
> to clarify one thing:
> 
> There is a minor change in behavior caused by switching from a
> `ThreadsListHandle::includes()` check to a `JavaThread::is_exiting()`
> check. The `JavaThread::is_exiting()` will return true before the target
> JavaThread is removed from the system's current ThreadsList. So the
> `JavaThread::is_exiting()` check will return true faster and the
> `ThreadsListHandle::includes()` check.
> 
> However, that change in behavior does not result in a change in
> behavior for a suspend thread request. Here's the relevant code:
> 
> 
> src/hotspot/share/runtime/thread.cpp:
> void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
> <snip>
> 
>     // The careful dance between thread suspension and exit is handled here.
>     // Since we are in thread_in_vm state and suspension is done with handshakes,
>     // we can just put in the exiting state and it will be correctly handled.
>     set_terminated(_thread_exiting);
> 
> <snip>
> 
>   // Remove from list of active threads list, and notify VM thread if we are the last non-daemon thread
>   Threads::remove(this, daemon);
> 
> <snip>
> }
> 
> 
> When we changed the suspend thread mechanism to use handshakes, we
> made it so that once the JavaThread called `set_terminated(_thread_exiting)`
> it no longer had to honor a suspend thread request.
> 
> Summary: Yes, the `is_exiting()` call will result in detecting the exiting
> JavaThread sooner than the `ThreadsListHandle::includes()` check.
> However, it will not result in a change in suspend thread behavior.

The `is_exiting` check seems unnecessary as the handshake code will not handshake with an exiting thread. The nested TLH was unnecessary too AFAICS.

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

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