Hi Dan,
On 18/02/2021 3:36 am, Daniel D.Daugherty wrote:
On Wed, 17 Feb 2021 07:32:46 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 three additional
commits since the last revision:
- Merge branch 'master' into JDK-8241403
- Address coleenp CR0 comments.
- 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware
Hi Dan,
Sorry but I have a lot of issues with this. After thinking about it a lot I
don't think the current approach is what is needed. To repeat what I wrote in
one of the comments I think the simple fix here is to replace the use of
Threads_lock in the caller with suitable use of a TLH, and then replace the
assert_locked_or_safepoint(Threads_lock) with
assert(is_JavaThread_protected(t)) where is_JavaThread_protected checks the
target for either being the current thread or included in a TLH of the current
thread. I don't think get_thread_name() should try to protect the target as
that is the responsibility of the caller.
Thanks,
David
@dholmes-ora - I'm really glad that I waited for your review! Thanks for taking
the time.
I think there are more issues with the existing code that then impact
any changes.
The general rule to operate on a target thread is that it must not be
able to terminate (or more strongly be deallocated) whilst you may
operate on it, but there are many ways this can be true:
- current thread
- at a safepoint
- in a handshake
- captured in a TLH
- target thread is immortal
- current thread holds a resource that the target needs before it exits
This last category is very general and can't really be captured by an
assertion. Holding the Threads_lock was one old example (and could be
asserted), but there are other possibilities including:
- current thread holds the ObjectMonitor of the target's j.l.Thread instance
- current thread is "synchronizing" with the target and the target can't
proceed until after the current thread releases it
So I think the current assertion in get_thread_name is more restrictive
than is actually necessary (imagine adding a logging statement in the OM
code that prints the name of the thread it has selected to unpark -
perfectly safe but no safepoint active and no Threads_lock held).
Changing that assertion is also more involved than just checking for an
active TLH as there are many safety conditions, not all of which can be
checked explicitly.
I think the CompileBroker code was a case of dealing with immortal
threads (before UseDynamicNumberOfCompilerThreads was introduced) and
the acquisition of the Threads_lock was simply done to placate the
assertion. With dynamic compiler threads it is I guess theoretically
possible that a newly created thread might terminate again before the
creating thread reaches the get_thread_name() call - but exceedingly
unlikely. Not sure how to fix that one without splitting apart the
thread creation and start processes.
Cheers,
David
-----
-------------
PR: https://git.openjdk.java.net/jdk/pull/2535
