On Sat, 16 Oct 2021 16:21:08 GMT, Daniel D. Daugherty <dcu...@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
