RFR: 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware [v5]

[Daniel D.Daugherty]

On Wed, 24 Feb 2021 00:36:28 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Daniel D. Daugherty has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8241403
>>  - Address dholmes-ora CR2 comments.
>>  - Address dholmes-ora CR1 comments.
>>  - Merge branch 'master' into JDK-8241403
>>  - Address coleenp CR0 comments.
>>  - 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware
>
> src/hotspot/share/compiler/compileBroker.cpp line 1115:
> 
>> 1113:       if (ct == NULL) break;
>> 1114:       ThreadsListHandle tlh;
>> 1115:       if (!tlh.includes(ct)) break;
> 
> This change raised a red flag because if the new thread already terminated it should decrement the number of compiler threads on its way out, but we are skipping the increment by breaking here. So that prompted me to look closer at the bigger picture for these compiler threads and their lifecycle. The current code is called whilst holding the CompilerThread_lock, which means that the new thread can't terminate because it has to also acquire the CompilerThread_lock to decide whether it needs to terminate. So access to ct is guaranteed to be safe and no TLH is needed at all - other than because get_thread_name() requires it. So this should be a simple assertion afterall.

Okay. I've switched this one back to an assertion after the TLH.
Thanks for the analysis.

> src/hotspot/share/compiler/compileBroker.cpp line 1136:
> 
>> 1134:       if (ct == NULL) break;
>> 1135:       ThreadsListHandle tlh;
>> 1136:       if (!tlh.includes(ct)) break;
> 
> Ditto.

Okay. I've switched this one back to an assertion after the TLH.

> src/hotspot/share/runtime/thread.cpp line 2955:
> 
>> 2953: void Threads::threads_do(JavaThreadIteratorWithHandle* jtiwh_p, ThreadClosure* tc) {
>> 2954:   java_threads_do(jtiwh_p, tc);
>> 2955:   non_java_threads_do(tc);
> 
> You should still have
> 
> (Threads_lock);```
> 
> for non-JavaThread access.

I'm backing out both:
Threads::java_threads_do(JavaThreadIteratorWithHandle* jtiwh_p, ThreadClosure* tc)
Threads::threads_do(JavaThreadIteratorWithHandle* jtiwh_p, ThreadClosure* tc)

and I'm backing out the changes to src/hotspot/share/services/management.cpp.
By removing the Threads_lock usage, I'm allowing those closures to run in parallel
with other closures that are using the original Threads::java_threads_do() and
Threads::threads_do() calls that do use the Threads_lock. Those other uses might
be assuming exclusivity for their algorithms and my M&M changes break that.

> src/hotspot/share/services/management.cpp line 1704:
> 
>> 1702:   {
>> 1703:     JavaThreadIteratorWithHandle jtiwh;
>> 1704:     Threads::threads_do(&jtiwh, &ttc);
> 
> Same comments as above re need for Threads_lock.

I'm backing out the changes to src/hotspot/share/services/management.cpp.

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

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