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

[Daniel D.Daugherty]

A minor fix to add a new function:

    bool Thread::is_JavaThread_protected(const JavaThread* p)

that returns true when the target JavaThread* is protected and false
otherwise. Update JavaThread::get_thread_name() to create a
ThreadsListHandle and use the new is_JavaThread_protected(). Also
update JvmtiTrace::safe_get_thread_name() to use the new
is_JavaThread_protected().

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

Commit messages:
 - 8241403: JavaThread::get_thread_name() should be ThreadSMR-aware

Changes: https://git.openjdk.java.net/jdk/pull/2535/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2535&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8241403
  Stats: 51 lines in 4 files changed: 26 ins; 9 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2535.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2535/head:pull/2535

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

[Daniel D.Daugherty]

On Thu, 11 Feb 2021 22:04:53 GMT, Daniel D. Daugherty <[hidden email]> wrote:

> A minor fix to add a new function:
>
>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>
> that returns true when the target JavaThread* is protected and false
> otherwise. Update JavaThread::get_thread_name() to create a
> ThreadsListHandle and use the new is_JavaThread_protected(). Also
> update JvmtiTrace::safe_get_thread_name() to use the new
> is_JavaThread_protected().

@dholmes-ora and @fisk - It looks like both of you are interested
in this fix. @robehn - This one is Thread-SMR related so you might
also want to take a look.

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

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

[Daniel D.Daugherty]

On Thu, 11 Feb 2021 22:07:22 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> A minor fix to add a new function:
>>
>>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>>
>> that returns true when the target JavaThread* is protected and false
>> otherwise. Update JavaThread::get_thread_name() to create a
>> ThreadsListHandle and use the new is_JavaThread_protected(). Also
>> update JvmtiTrace::safe_get_thread_name() to use the new
>> is_JavaThread_protected().
>
> @dholmes-ora and @fisk - It looks like both of you are interested
> in this fix. @robehn - This one is Thread-SMR related so you might
> also want to take a look.

@sspitsyn - I'm making a minor JVM/TI tracing tweak here so this
might also interest you.

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

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

[Robbin Ehn-2]

On Thu, 11 Feb 2021 22:04:53 GMT, Daniel D. Daugherty <[hidden email]> wrote:

> A minor fix to add a new function:
>
>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>
> that returns true when the target JavaThread* is protected and false
> otherwise. Update JavaThread::get_thread_name() to create a
> ThreadsListHandle and use the new is_JavaThread_protected(). Also
> update JvmtiTrace::safe_get_thread_name() to use the new
> is_JavaThread_protected().
>
> This fix is tested via a Mach5 Tier[1-8] run.

Seems alright.

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

Marked as reviewed by rehn (Reviewer).

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

[Coleen Phillimore-3]

On Thu, 11 Feb 2021 22:04:53 GMT, Daniel D. Daugherty <[hidden email]> wrote:

> A minor fix to add a new function:
>
>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>
> that returns true when the target JavaThread* is protected and false
> otherwise. Update JavaThread::get_thread_name() to create a
> ThreadsListHandle and use the new is_JavaThread_protected(). Also
> update JvmtiTrace::safe_get_thread_name() to use the new
> is_JavaThread_protected().
>
> This fix is tested via a Mach5 Tier[1-8] run.

This looks good, I have a change - could you see if you agree.
It's nice not to have to take Threads_lock to get the name of the thread.

src/hotspot/share/runtime/thread.hpp line 1692:

> 1690:   const char* get_thread_name() const;
> 1691:  protected:
> 1692:   friend class JvmtiTrace;  // so get_thread_name_string() can be called

I was trying to think of a way to not have JvmtiTrace not be a friend of JavaThread for this, maybe by adding a default value parameter to return "<NOT FILLED IN\>" rather than Thread::name.
is_JavaThread_protected only seems to be called by JvmtiTrace also, so should be private (with the friend, which also makes the friend unfortunate).

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

Changes requested by coleenp (Reviewer).

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

[Daniel D.Daugherty]

On Fri, 12 Feb 2021 22:02:18 GMT, Coleen Phillimore <[hidden email]> wrote:

>> A minor fix to add a new function:
>>
>>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>>
>> that returns true when the target JavaThread* is protected and false
>> otherwise. Update JavaThread::get_thread_name() to create a
>> ThreadsListHandle and use the new is_JavaThread_protected(). Also
>> update JvmtiTrace::safe_get_thread_name() to use the new
>> is_JavaThread_protected().
>>
>> This fix is tested via a Mach5 Tier[1-8] run.
>
> src/hotspot/share/runtime/thread.hpp line 1692:
>
>> 1690:   const char* get_thread_name() const;
>> 1691:  protected:
>> 1692:   friend class JvmtiTrace;  // so get_thread_name_string() can be called
>
> I was trying to think of a way to not have JvmtiTrace not be a friend of JavaThread for this, maybe by adding a default value parameter to return "<NOT FILLED IN\>" rather than Thread::name.
> is_JavaThread_protected only seems to be called by JvmtiTrace also, so should be private (with the friend, which also makes the friend unfortunate).

JavaThread::get_thread_name() also calls is_JavaThread_protected().

The "friend" is so that JvmtiTrace can call get_thread_name_string()
and we can get rid of the JvmtiTrace version of the logic. I kept the
"<NOT FILLED IN>" rather than figure out a way to call Thread::name()
so we don't introduce the possibility of a compatibility issue for any
code that might depend on that hand rolled string value...

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

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

[Coleen Phillimore-3]

On Fri, 12 Feb 2021 22:14:35 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> src/hotspot/share/runtime/thread.hpp line 1692:
>>
>>> 1690:   const char* get_thread_name() const;
>>> 1691:  protected:
>>> 1692:   friend class JvmtiTrace;  // so get_thread_name_string() can be called
>>
>> I was trying to think of a way to not have JvmtiTrace not be a friend of JavaThread for this, maybe by adding a default value parameter to return "<NOT FILLED IN\>" rather than Thread::name.
>> is_JavaThread_protected only seems to be called by JvmtiTrace also, so should be private (with the friend, which also makes the friend unfortunate).
>
> JavaThread::get_thread_name() also calls is_JavaThread_protected().
>
> The "friend" is so that JvmtiTrace can call get_thread_name_string()
> and we can get rid of the JvmtiTrace version of the logic. I kept the
> "<NOT FILLED IN>" rather than figure out a way to call Thread::name()
> so we don't introduce the possibility of a compatibility issue for any
> code that might depend on that hand rolled string value...

So Thread::is_JavaThread_protected() should be "protected" then, not public.
yes, I was suggesting adding a default last parameter like
    JavaThread::get_thread_name(char* default = Thread::name());
and pass "<NOT_FILLED_IN\>" from JVMTI. Then JVMTI doesn't have to be a friend and have more visibility to the JavaThread class than it should have.

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

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

[Serguei Spitsyn-2]

On Thu, 11 Feb 2021 22:04:53 GMT, Daniel D. Daugherty <[hidden email]> wrote:

> A minor fix to add a new function:
>
>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>
> that returns true when the target JavaThread* is protected and false
> otherwise. Update JavaThread::get_thread_name() to create a
> ThreadsListHandle and use the new is_JavaThread_protected(). Also
> update JvmtiTrace::safe_get_thread_name() to use the new
> is_JavaThread_protected().
>
> This fix is tested via a Mach5 Tier[1-8] run.

Hi Dan,
It looks good to me modulo your your discussion with Coleen on JvmtiTrace being a friend.
Thanks,
Serguei

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

Marked as reviewed by sspitsyn (Reviewer).

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

[Daniel D.Daugherty]

On Fri, 12 Feb 2021 23:37:40 GMT, Coleen Phillimore <[hidden email]> wrote:

>> JavaThread::get_thread_name() also calls is_JavaThread_protected().
>>
>> The "friend" is so that JvmtiTrace can call get_thread_name_string()
>> and we can get rid of the JvmtiTrace version of the logic. I kept the
>> "<NOT FILLED IN>" rather than figure out a way to call Thread::name()
>> so we don't introduce the possibility of a compatibility issue for any
>> code that might depend on that hand rolled string value...
>
> So Thread::is_JavaThread_protected() should be "protected" then, not public.
> yes, I was suggesting adding a default last parameter like
>     JavaThread::get_thread_name(char* default = Thread::name());
> and pass "<NOT_FILLED_IN\>" from JVMTI. Then JVMTI doesn't have to be a friend and have more visibility to the JavaThread class than it should have.

Ahhh.... I think I finally understand what you mean...
I'll look in the AM after I've had some coffee... :-)

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

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

[Daniel D.Daugherty]

On Sat, 13 Feb 2021 04:30:48 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> So Thread::is_JavaThread_protected() should be "protected" then, not public.
>> yes, I was suggesting adding a default last parameter like
>>     JavaThread::get_thread_name(char* default = Thread::name());
>> and pass "<NOT_FILLED_IN\>" from JVMTI. Then JVMTI doesn't have to be a friend and have more visibility to the JavaThread class than it should have.
>
> Ahhh.... I think I finally understand what you mean...
> I'll look in the AM after I've had some coffee... :-)

The name "default" is reserved so I went with "def_name".
You can't call Thread::name() from the declaration:
./open/src/hotspot/share/runtime/thread.hpp:1690:62: error: call to non-static member function without an object argument
  const char* get_thread_name(const char* def_name = Thread::name()) const;
                                                     ~~~~~~~~^~~~
so I went with setting `def_name = NULL` in the declaration and decoding that state in get_thread_name():
` return (def_name != NULL) ? def_name : Thread::name();`

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

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

[Coleen Phillimore-3]

On Sat, 13 Feb 2021 15:13:44 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Ahhh.... I think I finally understand what you mean...
>> I'll look in the AM after I've had some coffee... :-)
>
> The name "default" is reserved so I went with "def_name".
> You can't call Thread::name() from the declaration:
> ./open/src/hotspot/share/runtime/thread.hpp:1690:62: error: call to non-static member function without an object argument
>   const char* get_thread_name(const char* def_name = Thread::name()) const;
>                                                      ~~~~~~~~^~~~
> so I went with setting `def_name = NULL` in the declaration and decoding that state in get_thread_name():
> ` return (def_name != NULL) ? def_name : Thread::name();`

default_name, it's not long enough to abbreviate...  but yes. thanks!

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

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

[Daniel D.Daugherty]

On Sat, 13 Feb 2021 15:25:16 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> default_name, it's not long enough to abbreviate...  but yes. thanks!
>
> And it looks like making is_JavaThread_protected() a protected function doesn't work either since `JavaThread::get_thread_name()` can't call it:
>
> ./open/src/hotspot/share/runtime/thread.cpp:2857:15: error: 'is_JavaThread_protected' is a protected member of 'Thread'
>   if (thread->is_JavaThread_protected(this)) {
>               ^
> ./open/src/hotspot/share/runtime/thread.cpp:488:14: note: can only access this member on an object of type 'JavaThread'
> bool Thread::is_JavaThread_protected(const JavaThread* p) {
>              ^
> 1 error generated.

`JavaThread::get_thread_name()` is calling `is_JavaThread_protected()` on the calling Thread and NOT on the `this` JavaThread so `protected` doesn't work.

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

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

[Daniel D.Daugherty]

> A minor fix to add a new function:
>
>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>
> that returns true when the target JavaThread* is protected and false
> otherwise. Update JavaThread::get_thread_name() to create a
> ThreadsListHandle and use the new is_JavaThread_protected(). Also
> update JvmtiTrace::safe_get_thread_name() to use the new
> is_JavaThread_protected().
>
> This fix is tested via a Mach5 Tier[1-8] run.

Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:

  Address coleenp CR0 comments.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2535/files
  - new: https://git.openjdk.java.net/jdk/pull/2535/files/185434a8..00e38868

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2535&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2535&range=00-01

  Stats: 18 lines in 3 files changed: 2 ins; 6 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2535.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2535/head:pull/2535

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

[Daniel D.Daugherty]

On Sat, 13 Feb 2021 03:36:32 GMT, Serguei Spitsyn <[hidden email]> wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Address coleenp CR0 comments.
>
> Hi Dan,
> It looks good to me modulo your your discussion with Coleen on JvmtiTrace being a friend.
> Thanks,
> Serguei

Mach5 Tier[1-8] testing on CR1 is in process:
- Mach5 Tier[1-3] - no failures
- Mach5 Tier4 - 2 unrelated, known failures - still running
- Mach5 Tier5 - 1 unrelated, known failure
- Mach5 Tier6 - no failures - still running
- Mach5 Tier7 - just started
- Mach5 Tier8 - just started

@robehn, @coleenp, @sspitsyn - please take a look when you get the chance.

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

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

[Serguei Spitsyn-2]

On Sat, 13 Feb 2021 21:53:51 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> A minor fix to add a new function:
>>
>>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>>
>> that returns true when the target JavaThread* is protected and false
>> otherwise. Update JavaThread::get_thread_name() to create a
>> ThreadsListHandle and use the new is_JavaThread_protected(). Also
>> update JvmtiTrace::safe_get_thread_name() to use the new
>> is_JavaThread_protected().
>>
>> This fix is tested via a Mach5 Tier[1-8] run.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>
>   Address coleenp CR0 comments.

The update looks good.
Thanks,
Serguei

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

Marked as reviewed by sspitsyn (Reviewer).

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

[Coleen Phillimore-3]

On Sat, 13 Feb 2021 15:42:32 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> And it looks like making is_JavaThread_protected() a protected function doesn't work either since `JavaThread::get_thread_name()` can't call it:
>>
>> ./open/src/hotspot/share/runtime/thread.cpp:2857:15: error: 'is_JavaThread_protected' is a protected member of 'Thread'
>>   if (thread->is_JavaThread_protected(this)) {
>>               ^
>> ./open/src/hotspot/share/runtime/thread.cpp:488:14: note: can only access this member on an object of type 'JavaThread'
>> bool Thread::is_JavaThread_protected(const JavaThread* p) {
>>              ^
>> 1 error generated.
>
> `JavaThread::get_thread_name()` is calling `is_JavaThread_protected()` on the calling Thread and NOT on the `this` JavaThread so `protected` doesn't work.

ok.

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

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

[Coleen Phillimore-3]

On Sat, 13 Feb 2021 21:53:51 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> A minor fix to add a new function:
>>
>>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>>
>> that returns true when the target JavaThread* is protected and false
>> otherwise. Update JavaThread::get_thread_name() to create a
>> ThreadsListHandle and use the new is_JavaThread_protected(). Also
>> update JvmtiTrace::safe_get_thread_name() to use the new
>> is_JavaThread_protected().
>>
>> This fix is tested via a Mach5 Tier[1-8] run.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>
>   Address coleenp CR0 comments.

Thanks!

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

Marked as reviewed by coleenp (Reviewer).

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

[Daniel D.Daugherty]

On Tue, 16 Feb 2021 12:35:28 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Address coleenp CR0 comments.
>
> Thanks!

@coleenp and @sspitsyn - Thanks for the re-reviews!

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

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

[Daniel D.Daugherty]

On Tue, 16 Feb 2021 21:23:48 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Thanks!
>
> @coleenp and @sspitsyn - Thanks for the re-reviews!

@dholmes and @fisk - did either or both of you plan to review this one?

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

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

[Daniel D.Daugherty]

> A minor fix to add a new function:
>
>     bool Thread::is_JavaThread_protected(const JavaThread* p)
>
> that returns true when the target JavaThread* is protected and false
> otherwise. Update JavaThread::get_thread_name() to create a
> ThreadsListHandle and use the new is_JavaThread_protected(). Also
> update JvmtiTrace::safe_get_thread_name() to use the new
> is_JavaThread_protected().
>
> This fix is tested via a Mach5 Tier[1-8] run.

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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2535/files
  - new: https://git.openjdk.java.net/jdk/pull/2535/files/00e38868..1401c327

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2535&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2535&range=01-02

  Stats: 32041 lines in 979 files changed: 18959 ins; 8673 del; 4409 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2535.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2535/head:pull/2535

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