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

[Daniel D.Daugherty]

On Wed, 17 Feb 2021 07:07:44 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 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
>
> src/hotspot/share/runtime/thread.cpp line 490:
> 
>> 488:     // Current thread is always protected:
>> 489:     return true;
>> 490:   }
> 
> This is true but it is awkward that we are going to manifest Thread::current multiple times when using this - see below.

The static Thread::is_JavaThread_protected() now calls:

 bool Thread::is_JavaThread_protected(const JavaThread* p) {
  Thread* thread = Thread::current();
  if (thread == p) {

and because the function is now static, the caller doesn't have to call
Thread::current() and just calls Thread::is_JavaThread_protected(target).

> src/hotspot/share/runtime/thread.cpp line 2547:
> 
>> 2545: // for such that this method never returns NULL.
>> 2546: const char* JavaThread::get_thread_name(const char* default_name) const {
>> 2547:   Thread* thread = Thread::current();
> 
> So you manifest Thread::current() and then is_JavaThread_protected will manifest it again.

I'm dropping the call to Thread::current() in JavaThread::get_thread_name().

> src/hotspot/share/runtime/thread.cpp line 2548:
> 
>> 2546: const char* JavaThread::get_thread_name(const char* default_name) const {
>> 2547:   Thread* thread = Thread::current();
>> 2548:   ThreadsListHandle tlh(thread);
> 
> I would only expect this code to create a TLH if the target is not protected, otherwise why do we need to search through the existing thread-lists as it would suffice to see if the target is within the newly created TLH ??
> I expected the basic logic here to be:
> `if (thread is protected) return name;
> else { protect the thread; return name; }`
> 
> though if it is cheaper to create a TLH than to search the existing ones, then we may as well just create the TLH, check that the target has been captured and then return its name.
> 
> That said we need to carefully examine where the taking of the Threads_lock would occur in the thread termination sequence, to understand what observable states are possible with respect to the target having a threadObj. (Though this may have already changed with the introduction of Thread-SMR.)
> 
> That all said I'm not so sure the only thing we actually need to do for this issue is to replace the use of Threads_lock, in the caller, with suitable TLH usage, and then in get_thread_name() assert that the target is in fact protected.

I dropped the TLH into JavaThread::get_thread_name() because I was focused
on protecting the fetching of the name from the JavaThread. In reality that
doesn't happen in a vacuum so a TLH should really exist in the caller or some
where higher in the call stack.

There is a subtle point here: Creating a TLH in JavaThread::get_thread_name()
may not actually protect "this" JavaThread. "this" JavaThread would only be
protected by a newly created TLH, if it is on the current ThreadsList. If we
happen to call java_thread->get_thread_name() too early in the lifecycle or
too late in the lifecycle, then that newly created TLH doesn't help.

The original code's assertion check was a little complicated, but still was
a good idea:

    if we're not at a safepoint and if we're not calling the function on our self
       then assert_locked_or_safepoint_or_handshake(Threads_lock)

I'm going to add back a simpler assertion that takes advantage of the
fact that Thread::is_JavaThread_protected() returns true when the
target is the current thread.

> src/hotspot/share/prims/jvmtiTrace.cpp line 283:
> 
>> 281:   // If the target JavaThread is not protected, then we return the
>> 282:   // specified non-NULL string:
>> 283:   return thread->as_Java_thread()->get_thread_name("<NOT FILLED IN>");
> 
> That's not what the original logic is doing. We only return "NOT FILLED IN" if we find that the java.lang.Thread oop does not have a name set. That's not normally possible but this code is intended to be 100% safe, hence the check. (And it may be possible to see null during the attach process ...)
> 
> I would expect the target thread to already be protected when this method is called in any case. So while this code partially duplicates JavaThread::get_thread_name() I'm not convinced we need to change this code to use JavaThread::get_thread_name() instead.

My intent was to have safe_get_thread_name() take advantage of
JavaThread::get_thread_name() when we know that we're dealing
with a JavaThread. Shared code means less maintenance, but it
does look like I didn't achieve equivalence so I'm going to back
out my changes for JvmtiTrace::safe_get_thread_name().

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

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