On Wed, 24 Feb 2021 00:36:28 GMT, David Holmes <[email protected]> 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
