Re: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

[Daniil Titov]

Hi Serguei,

Please find below my answers to the concerns you mentioned in the previous 
email.

1. 
 > I have a concern about the checks for thread->is_exiting().
 > - the lines 632-633 are useless as they do not really protect from returning 
 > an exiting thread
> It is interesting what might happen if an exiting thread is returned by the
> ThreadsList::find_JavaThread_from_java_tid ().
> Does it make sense to develop a test that would cover these cases?

I agree, it doesn't really provide any protection so it makes sense just remove 
it. The current implementation 
find_JavaThread_from_java_tid()  doesn't provide such protection as well, since 
the thread could start exiting
immediately after method find_JavaThread_from_java_tid() returns, so the 
assumption is that the callers of
find_JavaThread_from_java_tid()  are expecting to deal with such threads and  
looking on some of them shows that
they usually try to retrieve threadObj or a thread statistic object and if it 
is NULL that just do nothing.

I'm not sure we could cover this specific case with the test. The window 
between find_JavaThread_from_java_tid() returns and the caller
continues the execution is too small. The window between the thread started 
exiting and removed itself from the thread table is very small as well.

2. 
>  - the lines 105-108 can result in adding exiting threads into the ThreadTable
 I agree, it was missed, we need to wrap this code inside Thread_lock in the 
similar way as it is done find_JavaThread_from_java_tid() 
 

3. 
> I would suggest to rewrite this fragment in a safe way:
>  95 {
>  96   MutexLocker ml(ThreadTableCreate_lock);
>  97   if (!_is_initialized) {
>  98 create_table(threads->length());
>  99 _is_initialized = true;
> 100   }
> 101 }
> as:
>   {
> MutexLocker ml(ThreadTableCreate_lock);
> if (_is_initialized) {
>   return;
 >}
 >create_table(threads->length());
 >_is_initialized = true;
 >  }

It was an intension to not block  while populating the table with the threads 
from the current thread list.  
There is no needs to have other threads that call 
find_JavaThread_from_java_tid()  be blocked and waiting for
 it to complete since the requested thread could be not present in the thread 
list that triggers the thread table
 initialization. Plus in case of racing initialization it allows threads from 
not original  thread lists be added to the table
and thus avoid the linear scan when these thread are looked up for the first 
time.

4. 
>> The case you have described is exact the reason why we still have a code 
>> inside
>> ThreadsList::find_JavaThread_from_java_tid() method that does a linear scan 
>> and adds
>>   the requested thread to the thread table if it is not there ( lines 
>> 614-613 below).

> I disagree because it is easy to avoid concurrent ThreadTable
> initialization (please, see my separate email).
> The reason for this code is to cover a case of late/lazy ThreadTable
> initialization.

David Holmes replied to this in a separate email providing a very detailed
explanation of the possible cases and how the proposed implementation satisfies 
them. 

Best regards,
Daniil

From: "serguei.spit...@oracle.com" 
Date: Tuesday, September 17, 2019 at 1:53 AM
To: Daniil Titov , Robbin Ehn 
, David Holmes , 
, OpenJDK Serviceability 
, "hotspot-runtime-...@openjdk.java.net" 
, "jmx-...@openjdk.java.net" 
, Claes Redestad 
Subject: Re: RFR: 8185005: Improve performance of 
ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

Hi Daniil,

Thank you for you patience in working on this issue!
Also, I like that the current thread related optimizations in management.cpp 
were factored out.
It was a good idea to separate them.

I have a concern about the checks for thread->is_exiting().
The threads are added to and removed from the ThreadTable under protection of 
Threads_lock.
However, the thread->is_exiting() checks are not protected, and so, they are 
racy.

There is a couple of such checks to mention:
 611 JavaThread* ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) 
const {
 612   ThreadTable::lazy_initialize(this);
 613   JavaThread* thread = ThreadTable::find_thread_by_tid(java_tid);
 614   if (thread == NULL) {
 615 // If the thread is not found in the table find it
 616 // with a linear search and add to the table.
 617 for (uint i = 0; i < length(); i++) {
 618   thread = thread_at(i);
 619   oop tobj = thread->threadObj();
 620   // Ignore the thread if it hasn't run yet, has exited
 621   // or is starting to exit.
 622   if (tobj != NULL && java_tid == java_lang_Thread::thread_id(tobj)) {
 623 MutexLocker ml(Threads_lock);
 624 // Must be inside the lock to ensure that we don't add the thread 
to the table
 625 // that has just passed the removal point in 
ThreadsSMRSupport::remove_thread()
 626 if (!thread->is_exiting()) {
 

Re: RFR: 8230857: Avoid reflection in sun.tools.common.ProcessHelper

[David Holmes]

Hi Erik,

Thanks for the additional details (I can't say I fully understand them :) ).

David

On 17/09/2019 11:39 pm, Erik Joelsson wrote:

Hello,

On 2019-09-17 05:59, David Holmes wrote:

Hi Magnus,

On 17/09/2019 9:26 pm, Magnus Ihse Bursie wrote:

On 2019-09-17 01:01, David Holmes wrote:

Hi Christoph,

Sorry for the delay getting back you.

cc'd build-dev to get some clarification on the below ...

On 12/09/2019 7:30 pm, Langer, Christoph wrote:

Hi David,


please review an enhancement which I've identified when working with
Processhelper for JDK-8230850.

I noticed that ProcessHelper is an interface in common code with a
static method that would lookup the actual platform 
implementation via
reflection. This seems a little cumbersome since we can have a 
common

dummy for ProcessHelper and override it with the platform specific
implementation, leveraging the build system.


I don't see you leveraging the build system. You have two source 
files

that compile to the same destination class file. What is ensuring the
platform specific version is compiled after the generic one?

Service-provider patterns use reflection to instantiate the service
implementation. I don't see any problem here that needs solving.


TL;DR:
There are two source files, one in share/classes and one in 
linux/classes. The build system overrides the share/classes 
implementation with the linux/classes implementation in the linux 
build. This is not by coincidence and only one class is contained 
in the generated jdk.jcmd module. Then there won't be a need for 
having a service interface and a service implementation that is 
looked up via reflection (which is not a bad pattern by itself). I 
agree that it's not a big problem to be solved but still not "no 
problem".
Here is some longer elaboration how the build system prefers 
specific implementations of classes and filters generic duplicates:
The SetupJavaCompilation function from JavaCompilation.gmk [0] is 
used to compile the java sources for JDK modules. In its 
documentation, for argument SRC [1], it claims: "one or more 
directories to search for sources. The order of the source roots is 
significant. The first found file of a certain name has priority". 
In its implementation the found files are first ordered [3] and 
duplicates filtered out [4].
The potential source files are handed to SetupJavaCompilation in 
CompileJavaModules.gmk [5] and were collected by a call to 
FindModuleSrcDirs [6]. FindModuleSrcDirs iterates over all 
potential source dirs for Java classes in the module [7]. The 
evaluated subdirs are (in that order) $(OPENJDK_TARGET_OS)/classes, 
$(OPENJDK_TARGET_OS_TYPE)/classes and share/classes, as per [8].

Hope that explains what I'm trying to leverage here.


I'm not 100% certain that what you describe actually ensures what 
you want it to ensure. I can't reconcile "the first found file ... 
has priority" with the fact found files are sorted and duplicates 
eliminated. It is the sorting that concerns me as it suggests 
linux/Foo.java might replace shared/Foo.java, but if we're on 
Windows then we have a problem! That said there is also this comment:


# Order src files according to the order of the src dirs. Correct 
odering is

# needed for correct overriding between different source roots.

I'd need the build team to clarify what "correct overriding" is 
actually defined as.

David,

Christoph is correct. linux/Foo.java will override share/Foo.java. I 
don't remember how the magic in JavaCompilation.gmk works anymore 
:-), but we have relied on this behavior in other places for a long 
time, so I'm pretty certain it is still working correctly. 
Presumably, the $(sort ...) is there to remove (identical) 
duplicates, which is a side-effect of sort.


Thanks for confirming. I'd still like to understand exactly what these 
overriding rules are though. It's not a mechanism I was aware of.


SetupJavaCompilation is indeed behaving as Christoph describes and it is 
by design. I implemented support for this behavior in:


https://bugs.openjdk.java.net/browse/JDK-8079344

The relevant parts of SetupJavaCompilation look like this:

   # Order src files according to the order of the src dirs. Correct 
odering is

   # needed for correct overriding between different source roots.
   $1_ALL_SRC_RAW := $$(call FindFiles, $$($1_SRC))
   $1_ALL_SRCS := $$($1_EXTRA_FILES) \
   $$(foreach d, $$($1_SRC), $$(filter $$d%, $$($1_ALL_SRC_RAW)))

The second line orders the src files by the src roots. (We used to just 
call find for one src root at a time, but the above actually performs 
better due only running 1 external process)


Further down we have this:

   ifneq ($$($1_KEEP_DUPS), true)
     # Remove duplicate source files by keeping the first found of each 
duplicate.
     # This allows for automatic overrides with custom or platform 
specific versions

     # source files.
     #
     # For the smart javac wrapper case, add each removed file to an 
extra exclude

Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

[David Holmes]

On 18/09/2019 12:10 am, Hohensee, Paul wrote:

Thanks, Serguei. :)

David, are you ok with the patch?


Yep, nothing further from me.

David


Paul

*From: *"serguei.spit...@oracle.com" 
*Date: *Tuesday, September 17, 2019 at 2:26 AM
*To: *"Hohensee, Paul" , David Holmes 
, Mandy Chung 
*Cc: *OpenJDK Serviceability , 
"hotspot-gc-...@openjdk.java.net" 
*Subject: *Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() 
can be quicker for self thread


Hi Paul,

Thank you for refactoring and fixing the test.
It looks great now!

Thanks,
Serguei


On 9/15/19 02:52, Hohensee, Paul wrote:

Hi, Serguei, thanks for the review. New webrev at

http://cr.openjdk.java.net/~phh/8207266/webrev.09/

I refactored the test’s main() method, and you’re correct,
getThreadAllocatedBytes should be getCurrentThreadAllocatedBytes in
that context: fixed.

Paul

*From: *"serguei.spit...@oracle.com"
 

*Organization: *Oracle Corporation
*Date: *Friday, September 13, 2019 at 5:50 PM
*To: *"Hohensee, Paul" 
, David Holmes 
, Mandy Chung
 
*Cc: *OpenJDK Serviceability 
,
"hotspot-gc-...@openjdk.java.net"



*Subject: *Re: RFR (M): 8207266:
ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

Hi Paul,

It looks pretty good in general.


http://cr.openjdk.java.net/~phh/8207266/webrev.08/test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java.frames.html

It would be nice to refactor the java main() method as it becomes
too big.
Two ways ofgetCurrentThreadAllocatedBytes() testing are good candidates
to become separate methods.

   98     long size1 = mbean.getThreadAllocatedBytes(id);

Just wanted to double check if you wanted to invoke
the getCurrentThreadAllocatedBytes() instead as it is
a part of:

   85 // First way, getCurrentThreadAllocatedBytes


Thanks,
Serguei

On 9/13/19 12:11 PM, Hohensee, Paul wrote:

Hi David, thanks for your comments. New webrev in

  


http://cr.openjdk.java.net/~phh/8207266/webrev.08/

  


Both the old and new versions of the code check that thread allocated 
memory is both supported and enabled. The existing version of 
getThreadAllocatedBytes(long []) calls verifyThreadAllocatedMemory(long []), 
which checks inline to make sure thread allocated memory is supported, then 
calls isThreadAllocatedMemoryEnabled() to verify that it's enabled. 
isThreadAllocatedMemoryEnabled() duplicates (!) the support check and returns 
the enabled flag. I removed the redundant check in the new version.

  


You're of course correct about the back-to-back check. Application code 
can't know when the runtime will hijack a thread for its own purposes. I've 
removed the check.

  


Paul

  


On 9/13/19, 12:50 AM, "David Holmes"  
  wrote:

  


     Hi Paul,

 


 On 13/09/2019 10:29 am, Hohensee, Paul wrote:

     > Thanks for clarifying the review rules. Would someone from the

 > serviceability team please review? New webrev at

     >

 >http://cr.openjdk.java.net/~phh/8207266/webrev.07/

 


 One aspect of the functional change needs clarification for me - 
and

 apologies if this has been covered in the past. It seems to me that

 currently we only check isThreadAllocatedMemorySupported for these

 operations, but if I read things correctly the updated code 
additionally

 checks isThreadAllocatedMemoryEnabled, which is a behaviour change 
not

 mentioned in the CSR.

 


 > I didn’t disturb the existing checks in the test, just added 
code to

 > check the result of getThreadAllocatedBytes(long) on a 
non-current

 > thread, plus the back-to-back no-allocation checks. The former 
wasn’t

 > needed before because getThreadAllocatedBytes(long) was just a 
wrapper

 > around getThreadAllocatedBytes(long []). This patch changes 
that, so I

 > added a separate test. The latter is supposed to fail if there’s 
object

 > allocation on calls to getCurrentThreadAllocatedBytes and

 > getThreadAllocatedBytes(long). I.e., a feature, not a bug, 
because

 > accumulation of transient small objects can be a performance 
problem.

 > Thanks to your review, I noticed that the back-to-back check on 
the

 > current thread was using 

Re: RFR: 8230857: Avoid reflection in sun.tools.common.ProcessHelper

[Erik Joelsson]

Hello,

On 2019-09-17 05:59, David Holmes wrote:

Hi Magnus,

On 17/09/2019 9:26 pm, Magnus Ihse Bursie wrote:

On 2019-09-17 01:01, David Holmes wrote:

Hi Christoph,

Sorry for the delay getting back you.

cc'd build-dev to get some clarification on the below ...

On 12/09/2019 7:30 pm, Langer, Christoph wrote:

Hi David,


please review an enhancement which I've identified when working with
Processhelper for JDK-8230850.

I noticed that ProcessHelper is an interface in common code with a
static method that would lookup the actual platform 
implementation via
reflection. This seems a little cumbersome since we can have a 
common

dummy for ProcessHelper and override it with the platform specific
implementation, leveraging the build system.


I don't see you leveraging the build system. You have two source 
files

that compile to the same destination class file. What is ensuring the
platform specific version is compiled after the generic one?

Service-provider patterns use reflection to instantiate the service
implementation. I don't see any problem here that needs solving.


TL;DR:
There are two source files, one in share/classes and one in 
linux/classes. The build system overrides the share/classes 
implementation with the linux/classes implementation in the linux 
build. This is not by coincidence and only one class is contained 
in the generated jdk.jcmd module. Then there won't be a need for 
having a service interface and a service implementation that is 
looked up via reflection (which is not a bad pattern by itself). I 
agree that it's not a big problem to be solved but still not "no 
problem".
Here is some longer elaboration how the build system prefers 
specific implementations of classes and filters generic duplicates:
The SetupJavaCompilation function from JavaCompilation.gmk [0] is 
used to compile the java sources for JDK modules. In its 
documentation, for argument SRC [1], it claims: "one or more 
directories to search for sources. The order of the source roots is 
significant. The first found file of a certain name has priority". 
In its implementation the found files are first ordered [3] and 
duplicates filtered out [4].
The potential source files are handed to SetupJavaCompilation in 
CompileJavaModules.gmk [5] and were collected by a call to 
FindModuleSrcDirs [6]. FindModuleSrcDirs iterates over all 
potential source dirs for Java classes in the module [7]. The 
evaluated subdirs are (in that order) $(OPENJDK_TARGET_OS)/classes, 
$(OPENJDK_TARGET_OS_TYPE)/classes and share/classes, as per [8].

Hope that explains what I'm trying to leverage here.


I'm not 100% certain that what you describe actually ensures what 
you want it to ensure. I can't reconcile "the first found file ... 
has priority" with the fact found files are sorted and duplicates 
eliminated. It is the sorting that concerns me as it suggests 
linux/Foo.java might replace shared/Foo.java, but if we're on 
Windows then we have a problem! That said there is also this comment:


# Order src files according to the order of the src dirs. Correct 
odering is

# needed for correct overriding between different source roots.

I'd need the build team to clarify what "correct overriding" is 
actually defined as.

David,

Christoph is correct. linux/Foo.java will override share/Foo.java. I 
don't remember how the magic in JavaCompilation.gmk works anymore 
:-), but we have relied on this behavior in other places for a long 
time, so I'm pretty certain it is still working correctly. 
Presumably, the $(sort ...) is there to remove (identical) 
duplicates, which is a side-effect of sort.


Thanks for confirming. I'd still like to understand exactly what these 
overriding rules are though. It's not a mechanism I was aware of.


SetupJavaCompilation is indeed behaving as Christoph describes and it is 
by design. I implemented support for this behavior in:


https://bugs.openjdk.java.net/browse/JDK-8079344

The relevant parts of SetupJavaCompilation look like this:

  # Order src files according to the order of the src dirs. Correct 
odering is

  # needed for correct overriding between different source roots.
  $1_ALL_SRC_RAW := $$(call FindFiles, $$($1_SRC))
  $1_ALL_SRCS := $$($1_EXTRA_FILES) \
  $$(foreach d, $$($1_SRC), $$(filter $$d%, $$($1_ALL_SRC_RAW)))

The second line orders the src files by the src roots. (We used to just 
call find for one src root at a time, but the above actually performs 
better due only running 1 external process)


Further down we have this:

  ifneq ($$($1_KEEP_DUPS), true)
    # Remove duplicate source files by keeping the first found of each 
duplicate.
    # This allows for automatic overrides with custom or platform 
specific versions

    # source files.
    #
    # For the smart javac wrapper case, add each removed file to an 
extra exclude

    # file list to prevent sjavac from finding duplicate sources.
    $1_SRCS := $$(strip $$(foreach s, $$($1_SRCS), \
    $$(eval relative_src := 

Re: RFR: 8230857: Avoid reflection in sun.tools.common.ProcessHelper

[David Holmes]

Hi Magnus,

On 17/09/2019 9:26 pm, Magnus Ihse Bursie wrote:

On 2019-09-17 01:01, David Holmes wrote:

Hi Christoph,

Sorry for the delay getting back you.

cc'd build-dev to get some clarification on the below ...

On 12/09/2019 7:30 pm, Langer, Christoph wrote:

Hi David,


please review an enhancement which I've identified when working with
Processhelper for JDK-8230850.

I noticed that ProcessHelper is an interface in common code with a
static method that would lookup the actual platform implementation via
reflection. This seems a little cumbersome since we can have a common
dummy for ProcessHelper and override it with the platform specific
implementation, leveraging the build system.


I don't see you leveraging the build system. You have two source files
that compile to the same destination class file. What is ensuring the
platform specific version is compiled after the generic one?

Service-provider patterns use reflection to instantiate the service
implementation. I don't see any problem here that needs solving.


TL;DR:
There are two source files, one in share/classes and one in 
linux/classes. The build system overrides the share/classes 
implementation with the linux/classes implementation in the linux 
build. This is not by coincidence and only one class is contained in 
the generated jdk.jcmd module. Then there won't be a need for having 
a service interface and a service implementation that is looked up 
via reflection (which is not a bad pattern by itself). I agree that 
it's not a big problem to be solved but still not "no problem".
Here is some longer elaboration how the build system prefers specific 
implementations of classes and filters generic duplicates:
The SetupJavaCompilation function from JavaCompilation.gmk [0] is 
used to compile the java sources for JDK modules. In its 
documentation, for argument SRC [1], it claims: "one or more 
directories to search for sources. The order of the source roots is 
significant. The first found file of a certain name has priority". In 
its implementation the found files are first ordered [3] and 
duplicates filtered out [4].
The potential source files are handed to SetupJavaCompilation in 
CompileJavaModules.gmk [5] and were collected by a call to 
FindModuleSrcDirs [6].  FindModuleSrcDirs iterates over all potential 
source dirs for Java classes in the module [7]. The evaluated subdirs 
are (in that order) $(OPENJDK_TARGET_OS)/classes, 
$(OPENJDK_TARGET_OS_TYPE)/classes and share/classes, as per [8].

Hope that explains what I'm trying to leverage here.


I'm not 100% certain that what you describe actually ensures what you 
want it to ensure. I can't reconcile "the first found file ... has 
priority" with the fact found files are sorted and duplicates 
eliminated. It is the sorting that concerns me as it suggests 
linux/Foo.java might replace shared/Foo.java, but if we're on Windows 
then we have a problem! That said there is also this comment:


# Order src files according to the order of the src dirs. Correct 
odering is

# needed for correct overriding between different source roots.

I'd need the build team to clarify what "correct overriding" is 
actually defined as.

David,

Christoph is correct. linux/Foo.java will override share/Foo.java. I 
don't remember how the magic in JavaCompilation.gmk works anymore :-), 
but we have relied on this behavior in other places for a long time, so 
I'm pretty certain it is still working correctly. Presumably, the $(sort 
...) is there to remove (identical) duplicates, which is a side-effect 
of sort.


Thanks for confirming. I'd still like to understand exactly what these 
overriding rules are though. It's not a mechanism I was aware of.


Thanks,
David


/Magnus



Thanks,
David
-

I've uploaded an updated webrev which contains some cleanup to the 
Test changes: http://cr.openjdk.java.net/~clanger/webrevs/8230857.1/


Thanks
Christoph

[0] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l185 

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l157 

[3] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l225 

[4] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l257 

[5] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l603 

[6] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l555 

[7] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l300 

[8] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l243 

Re: RFR: 8230857: Avoid reflection in sun.tools.common.ProcessHelper

[Magnus Ihse Bursie]

On 2019-09-17 01:01, David Holmes wrote:

Hi Christoph,

Sorry for the delay getting back you.

cc'd build-dev to get some clarification on the below ...

On 12/09/2019 7:30 pm, Langer, Christoph wrote:

Hi David,


please review an enhancement which I've identified when working with
Processhelper for JDK-8230850.

I noticed that ProcessHelper is an interface in common code with a
static method that would lookup the actual platform implementation via
reflection. This seems a little cumbersome since we can have a common
dummy for ProcessHelper and override it with the platform specific
implementation, leveraging the build system.


I don't see you leveraging the build system. You have two source files
that compile to the same destination class file. What is ensuring the
platform specific version is compiled after the generic one?

Service-provider patterns use reflection to instantiate the service
implementation. I don't see any problem here that needs solving.


TL;DR:
There are two source files, one in share/classes and one in 
linux/classes. The build system overrides the share/classes 
implementation with the linux/classes implementation in the linux 
build. This is not by coincidence and only one class is contained in 
the generated jdk.jcmd module. Then there won't be a need for having 
a service interface and a service implementation that is looked up 
via reflection (which is not a bad pattern by itself). I agree that 
it's not a big problem to be solved but still not "no problem".
Here is some longer elaboration how the build system prefers specific 
implementations of classes and filters generic duplicates:
The SetupJavaCompilation function from JavaCompilation.gmk [0] is 
used to compile the java sources for JDK modules. In its 
documentation, for argument SRC [1], it claims: "one or more 
directories to search for sources. The order of the source roots is 
significant. The first found file of a certain name has priority". In 
its implementation the found files are first ordered [3] and 
duplicates filtered out [4].
The potential source files are handed to SetupJavaCompilation in 
CompileJavaModules.gmk [5] and were collected by a call to 
FindModuleSrcDirs [6].  FindModuleSrcDirs iterates over all potential 
source dirs for Java classes in the module [7]. The evaluated subdirs 
are (in that order) $(OPENJDK_TARGET_OS)/classes, 
$(OPENJDK_TARGET_OS_TYPE)/classes and share/classes, as per [8].

Hope that explains what I'm trying to leverage here.


I'm not 100% certain that what you describe actually ensures what you 
want it to ensure. I can't reconcile "the first found file ... has 
priority" with the fact found files are sorted and duplicates 
eliminated. It is the sorting that concerns me as it suggests 
linux/Foo.java might replace shared/Foo.java, but if we're on Windows 
then we have a problem! That said there is also this comment:


# Order src files according to the order of the src dirs. Correct 
odering is

# needed for correct overriding between different source roots.

I'd need the build team to clarify what "correct overriding" is 
actually defined as.

David,

Christoph is correct. linux/Foo.java will override share/Foo.java. I 
don't remember how the magic in JavaCompilation.gmk works anymore :-), 
but we have relied on this behavior in other places for a long time, so 
I'm pretty certain it is still working correctly. Presumably, the $(sort 
...) is there to remove (identical) duplicates, which is a side-effect 
of sort.


/Magnus



Thanks,
David
-

I've uploaded an updated webrev which contains some cleanup to the 
Test changes: http://cr.openjdk.java.net/~clanger/webrevs/8230857.1/


Thanks
Christoph

[0] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l185
[1] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l157
[3] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l225
[4] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l257
[5] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l603
[6] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l555
[7] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l300
[8] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l243

RE: RFR [XS]: 8230901: missing ReleaseStringUTFChars in servicability native code

[Baesken, Matthias]

Hi  Serguei and Thomas , thanks for the reviews.

>Should I open a bug for these ?
> Probably, two different bug are needed: hotspot/runtime and AWT.

Regarding  the atoi  on input provided by getenv -  I’ll open  2 bugs for this.

Best regards, Matthias



From: serguei.spit...@oracle.com 
Sent: Samstag, 14. September 2019 00:18
To: Baesken, Matthias ; Thomas Stüfe 

Cc: serviceability-dev@openjdk.java.net
Subject: Re: RFR [XS]: 8230901: missing ReleaseStringUTFChars in servicability 
native code

Hi Matthias,

On 9/12/19 4:52 AM, Baesken, Matthias wrote:
Hi Thomas,   thanks for the review .

You are correct about atoi .

New webrev  :

http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.1/


I had 2 additional  observations  :


  1.  With  OJDK on solaris 32bit gone for quite some time, we might be  able  
to kick out the whole  non _LP64  code  because we are always 64 bit

(maybe  someone could comment if this is a safe assumption,  there might be old 
32bit solaris core files flying around for some reason even these days … )

http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.1/src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp.frames.html

696   // some older versions of libproc.so crash when trying to attach 32 bit
697   // debugger to 64 bit core file. check and throw error.
698 #ifndef _LP64
…..


  1.  The usage of atoi is commented  here :

https://docs.oracle.com/cd/E86824_01/html/E54766/atoi-3c.html

„However, applications should not use the atoi(), atol(), or atoll() functions 
unless they know the value represented by the argument will be in range for the 
corresponding result type”

  ……And here :

https://pubs.opengroup.org/onlinepubs/009695399/functions/atoi.html

“If the number is not known to be in range, 
strtol() 
should be used because atoi() is not required to perform any error checking”

However  we  have  a number of  usages in the coding   where  atoi is called 
without knowing that  the  argument is in the allowed range .

some examples :

src/hotspot/share/runtime/arguments.cpp-382-if (match_option(option, 
"-Dsun.java.launcher.pid=", )) {
src/hotspot/share/runtime/arguments.cpp:383:  _sun_java_launcher_pid = 
atoi(tail);
src/hotspot/share/runtime/arguments.cpp-384-  continue;


src/java.desktop/unix/native/libawt_xawt/xawt/XToolkit.c
455value = getenv("_AWT_MAX_POLL_TIMEOUT");
456if (value != NULL) {
457AWT_MAX_POLL_TIMEOUT = atoi(value);


src/java.desktop/unix/native/common/awt/X11Color.c-781-if 
(getenv("CMAPSIZE") != 0) {
src/java.desktop/unix/native/common/awt/X11Color.c:782:cmapsize = 
atoi(getenv("CMAPSIZE"));


Should I open a bug for these ?

Probably, two different bug are needed: hotspot/runtime and AWT.

Thanks,
Serguei



Best regards, Matthias





From: Thomas Stüfe 
Sent: Donnerstag, 12. September 2019 12:22
To: Baesken, Matthias 

Cc: 
serviceability-dev@openjdk.java.net
Subject: Re: RFR [XS]: 8230901: missing ReleaseStringUTFChars in servicability 
native code

Hi Matthias,

your changes look good.

an additional bug:

http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.0/src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp.frames.html

 698 #ifndef _LP64
 699   atoi(cmdLine_cstr);
 700   if (errno) {

Behaviour of atoi() in error case is undefined. errno values are not defined.

See: https://pubs.opengroup.org/onlinepubs/009695399/functions/atoi.html

And even if atoi would set errno, this is still not enough since errno may 
contain a stale value. One would have to set errno=0 before the function call.

If you want to fix this too 'd suggest replacing this call with strtol().

Cheers, Thomas

On Thu, Sep 12, 2019 at 12:11 PM Baesken, Matthias 
mailto:matthias.baes...@sap.com>> wrote:
Hello, please reviews this small change .

It adds  ReleaseStringUTFChars  calls  at some places in early return cases .
( in src/jdk.hotspot.agent/solaris/native/libsaproc/saproc.cpp

THROW_NEW_DEBUGGER_EXCEPTION  contains a return , see the macro declaration


39 #define THROW_NEW_DEBUGGER_EXCEPTION(str) { throwNewDebuggerException(env, 
str); return;}
)



Bug/webrev :

https://bugs.openjdk.java.net/browse/JDK-8230901

http://cr.openjdk.java.net/~mbaesken/webrevs/8230901.0/

Thanks, Matthias

Re: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

[David Holmes]

Hi Serguei,

On 17/09/2019 7:10 pm, serguei.spit...@oracle.com wrote:

Hi Daniil,


On 9/16/19 21:36, Daniil Titov wrote:

Hi David,

The case you have described is exact the reason why we still have a 
code inside
ThreadsList::find_JavaThread_from_java_tid() method that does a linear 
scan and adds
  the requested thread to the thread table if it is not there ( lines 
614-613 below).


I disagree because it is easy to avoid concurrent ThreadTable 
initialization (please, see my separate email).
The reason for this code is to cover a case of late/lazy ThreadTable 
initialization.


I'm not sure I follow. With the current code if two threads are racing 
to initialize the ThreadTable with ThreadsLists that contain a different 
set of threads then there are two possibilities with regards to the 
interleaving. Assume T1 initializes the table with its set of threads 
and so finds the tid it is looking for in the table. Meanwhile T2 is 
racing with the initialization logic:


- If T2 sees _is_initialized then lazy_initialization does nothing for 
T2, and the additional threads in its ThreadsList (say T3 and T4) are 
not added to the table. But the specific thread associated with the tid 
(say T3) will be found by linear search of the ThreadsList and then 
added. If any other threads come searching for T4 they too will not find 
it in the ThreadTable but instead perform the linear search of their 
ThreadsList (and add it).


- if T2 doesn't see _is_initialized at first it will try to acquire the 
lock, and eventually see _is_initialized is true, at which point it will 
try to add all of its thread's to the table (so T3 and T4 will be 
added). When lazy_initialize returns, T3 will be found in the table and 
returned. If any other threads come searching for T4 they will also find 
it in the table.


With your suggested code change this second case is not possible so for 
any racing initialization the lookup of any threads not in the original 
ThreadsList will always result in using the linear search before adding 
to the table.


Both seem correct to me. Which one is more efficient will totally depend 
on the number of differences between the ThreadsLists and whether the 
code ever tries to look up those additional threads. If we assume racing 
initialization is likely to be rare anyway (because generally one thread 
is in charge of doing the monitoring) then the choice seems somewhat 
arbitrary.


Cheers,
David
-


Thanks,
Serguei


    The
assumption is that it's quite uncommon and even if this is the case 
the linear scan happens

only once per such thread.

  611 JavaThread* ThreadsList::find_JavaThread_from_java_tid(jlong 
java_tid) const {

  612   ThreadTable::lazy_initialize(this);
  613   JavaThread* thread = ThreadTable::find_thread_by_tid(java_tid);
  614   if (thread == NULL) {
  615 // If the thread is not found in the table find it
  616 // with a linear search and add to the table.
  617 for (uint i = 0; i < length(); i++) {
  618   thread = thread_at(i);
  619   oop tobj = thread->threadObj();
  620   // Ignore the thread if it hasn't run yet, has exited
  621   // or is starting to exit.
  622   if (tobj != NULL && java_tid == 
java_lang_Thread::thread_id(tobj)) {

  623 MutexLocker ml(Threads_lock);
  624 // Must be inside the lock to ensure that we don't add 
the thread to the table
  625 // that has just passed the removal point in 
ThreadsSMRSupport::remove_thread()

  626 if (!thread->is_exiting()) {
  627   ThreadTable::add_thread(java_tid, thread);
  628   return thread;
  629 }
  630   }
  631 }
  632   } else if (!thread->is_exiting()) {
  633   return thread;
  634   }
  635   return NULL;
  636 }

Thanks,
Daniil

On 9/16/19, 7:27 PM, "David Holmes"  wrote:

 Hi Daniil,
 Thanks again for your perseverance on this one.
 I think there is a problem with initialization of the thread table.
 Suppose thread T1 has called 
ThreadsList::find_JavaThread_from_java_tid
 and has commenced execution of ThreadTable::lazy_initialize, but 
not yet

 marked _is_initialized as true. Now two new threads (T2 and T3) are
 created and start running - they aren't added to the ThreadTable yet
 because it isn't initialized. Now T0 also calls
 ThreadsList::find_JavaThread_from_java_tid using an updated 
ThreadsList
 that contains T2 and T3. It also calls 
ThreadTable::lazy_initialize. If
 _is_initialized is still false T0 will attempt initialization but 
once
 it gets the lock it will see the table has now been initialized 
by T1.
 It will then proceed to update the table with its own ThreadList 
content
 - adding T2 and T3. That is all fine. But now suppose T0 
initially sees

 _is_initialized as true, it will do nothing in lazy_initialize and
 simply return to find_JavaThread_from_java_tid. But now T2 and T3 
are
 missing from the 

Re: RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

[serguei.spit...@oracle.com]

  
  
Hi Paul,
  
  Thank you for refactoring and fixing the test.
  It looks great now!
  
  Thanks,
  Serguei
  
  
  On 9/15/19 02:52, Hohensee, Paul wrote:


  
  
Hi, Serguei, thanks for the review. New
  webrev at
 
http://cr.openjdk.java.net/~phh/8207266/webrev.09/
 
I refactored the test’s main() method, and
  you’re correct, getThreadAllocatedBytes should be
  getCurrentThreadAllocatedBytes in that context: fixed.
 
Paul
 

  From: "serguei.spit...@oracle.com"
  
  Organization: Oracle Corporation
  Date: Friday, September 13, 2019 at 5:50 PM
  To: "Hohensee, Paul" ,
  David Holmes , Mandy Chung
  
  Cc: OpenJDK Serviceability
  ,
  "hotspot-gc-...@openjdk.java.net"
  
  Subject: Re: RFR (M): 8207266:
  ThreadMXBean::getThreadAllocatedBytes() can be quicker for
  self thread


   

Hi Paul,
  
  It looks pretty good in general.
  
  http://cr.openjdk.java.net/~phh/8207266/webrev.08/test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java.frames.html
  
  It would be nice to refactor the java main() method as it
  becomes too big.
  Two ways of
getCurrentThreadAllocatedBytes() testing are good
  candidates
  to become separate methods.
  98     long size1 = mbean.getThreadAllocatedBytes(id);
Just wanted to double check if you wanted
  to invoke
  the getCurrentThreadAllocatedBytes()
instead as it is
  a part of:
  85 // First way, getCurrentThreadAllocatedBytes

  Thanks,
  Serguei

  On 9/13/19 12:11 PM, Hohensee, Paul
wrote:


  Hi David, thanks for your comments. New webrev in
   
  http://cr.openjdk.java.net/~phh/8207266/webrev.08/
   
  Both the old and new versions of the code check that thread allocated memory is both supported and enabled. The existing version of getThreadAllocatedBytes(long []) calls verifyThreadAllocatedMemory(long []), which checks inline to make sure thread allocated memory is supported, then calls isThreadAllocatedMemoryEnabled() to verify that it's enabled. isThreadAllocatedMemoryEnabled() duplicates (!) the support check and returns the enabled flag. I removed the redundant check in the new version.
   
  You're of course correct about the back-to-back check. Application code can't know when the runtime will hijack a thread for its own purposes. I've removed the check.
   
  Paul
   
  On 9/13/19, 12:50 AM, "David Holmes"  wrote:
   
      Hi Paul,
      
  On 13/09/2019 10:29 am, Hohensee, Paul wrote:
      > Thanks for clarifying the review rules. Would someone from the 
  > serviceability team please review? New webrev at
      > 
  > http://cr.openjdk.java.net/~phh/8207266/webrev.07/
      
  One aspect of the functional change needs clarification for me - and 
  apologies if this has been covered in the past. It seems to me that 
  currently we only check isThreadAllocatedMemorySupported for these 
  operations, but if I read things correctly the updated code additionally 
  checks isThreadAllocatedMemoryEnabled, which is a behaviour change not 
  mentioned in the CSR.
      
  > I didn’t disturb the existing checks in the test, just added code to 
  > check the result of getThreadAllocatedBytes(long) on a non-current 
  > thread, plus the back-to-back no-allocation checks. The former wasn’t 
  > needed before because getThreadAllocatedBytes(long) was just a wrapper 
  > around getThreadAllocatedBytes(long []). This patch changes that, so I 
  > added a separate test. The latter is supposed to fail if there’s object 
  > allocation on calls to getCurrentThreadAllocatedBytes and 
  > getThreadAllocatedBytes(long). I.e., a feature, not a bug, because 
  > accumulation of transient small objects can be a performance problem. 
  > Thanks to your review, I noticed that the back-to-back check on the 
  > current thread was using getThreadAllocatedBytes(long) instead of 
  > getCurrentThreadAllocatedBytes and fixed it. I also removed all 
  > instances of “TEST FAILED: “.
      
  The back-to-back check is not valid in general. You don't know if the 
  

Re: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

[serguei.spit...@oracle.com]

Hi Daniil,


On 9/16/19 21:36, Daniil Titov wrote:

Hi David,

The case you have described is exact the reason why we still have a code inside
ThreadsList::find_JavaThread_from_java_tid() method that does a linear scan and 
adds
  the requested thread to the thread table if it is not there ( lines 614-613 
below).


I disagree because it is easy to avoid concurrent ThreadTable 
initialization (please, see my separate email).
The reason for this code is to cover a case of late/lazy ThreadTable 
initialization.


Thanks,
Serguei


The
assumption is that it's quite uncommon and even if this is the case the linear 
scan happens
only once per such thread.

 
  611 JavaThread* ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) const {

  612   ThreadTable::lazy_initialize(this);
  613   JavaThread* thread = ThreadTable::find_thread_by_tid(java_tid);
  614   if (thread == NULL) {
  615 // If the thread is not found in the table find it
  616 // with a linear search and add to the table.
  617 for (uint i = 0; i < length(); i++) {
  618   thread = thread_at(i);
  619   oop tobj = thread->threadObj();
  620   // Ignore the thread if it hasn't run yet, has exited
  621   // or is starting to exit.
  622   if (tobj != NULL && java_tid == java_lang_Thread::thread_id(tobj)) {
  623 MutexLocker ml(Threads_lock);
  624 // Must be inside the lock to ensure that we don't add the thread 
to the table
  625 // that has just passed the removal point in 
ThreadsSMRSupport::remove_thread()
  626 if (!thread->is_exiting()) {
  627   ThreadTable::add_thread(java_tid, thread);
  628   return thread;
  629 }
  630   }
  631 }
  632   } else if (!thread->is_exiting()) {
  633   return thread;
  634   }
  635   return NULL;
  636 }

Thanks,
Daniil

On 9/16/19, 7:27 PM, "David Holmes"  wrote:

 Hi Daniil,
 
 Thanks again for your perseverance on this one.
 
 I think there is a problem with initialization of the thread table.

 Suppose thread T1 has called ThreadsList::find_JavaThread_from_java_tid
 and has commenced execution of ThreadTable::lazy_initialize, but not yet
 marked _is_initialized as true. Now two new threads (T2 and T3) are
 created and start running - they aren't added to the ThreadTable yet
 because it isn't initialized. Now T0 also calls
 ThreadsList::find_JavaThread_from_java_tid using an updated ThreadsList
 that contains T2 and T3. It also calls ThreadTable::lazy_initialize. If
 _is_initialized is still false T0 will attempt initialization but once
 it gets the lock it will see the table has now been initialized by T1.
 It will then proceed to update the table with its own ThreadList content
 - adding T2 and T3. That is all fine. But now suppose T0 initially sees
 _is_initialized as true, it will do nothing in lazy_initialize and
 simply return to find_JavaThread_from_java_tid. But now T2 and T3 are
 missing from the ThreadTable and nothing will cause them to be added.
 
 More generally any ThreadsList that is created after the ThreadsList

 that will be used for initialization, may contain threads that will not
 be added to the table.
 
 Thanks,

 David
 
 On 17/09/2019 4:18 am, Daniil Titov wrote:

 > Hello,
 >
 > After investigating with Claes the impact of this change on the 
performance (thanks a lot Claes for helping with it!) the conclusion was that the 
impact on the thread startup time is not a blocker for this change.
 >
 > I also measured the memory footprint using Native Memory Tracking and 
results showed around 40 bytes per live thread.
 >
 > Please review a new version of the fix, webrev.06 [1].  Just to remind,  
webrev.05 was abandoned and webrev.06 [1] is webrev.04 [3] minus changes in 
src/hotspot/share/services/management.cpp (that were factored out to a separate 
issue [4]) and plus a change in ThreadsList::find_JavaThread_from_java_tid() 
method (please, see below)  that addresses the problem Robbin found and puts the 
code that adds a new thread to the thread table inside Threads_lock.
 >
 > src/hotspot/share/runtime/threadSMR.cpp
 >
 > 622   if (tobj != NULL && java_tid == 
java_lang_Thread::thread_id(tobj)) {
 > 623 MutexLocker ml(Threads_lock);
 > 624 // Must be inside the lock to ensure that we don't add the 
thread to the table
 > 625 // that has just passed the removal point in 
ThreadsSMRSupport::remove_thread()
 > 626 if (!thread->is_exiting()) {
 > 627   ThreadTable::add_thread(java_tid, thread);
 > 628   return thread;
 > 629 }
 > 630   }
 >
 > [1] Webrev:  https://cr.openjdk.java.net/~dtitov/8185005/webrev.06
 > [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
 > [3] 

Re: RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

[serguei.spit...@oracle.com]

  
  
Hi David,
  
  It is a nice catch!
  I would suggest to rewrite this fragment in a safe way:
95 {
  96   MutexLocker ml(ThreadTableCreate_lock);
  97   if (!_is_initialized) {
  98 create_table(threads->length());
  99 _is_initialized = true;
 100   }
 101 }


  as:
 {
 MutexLocker ml(ThreadTableCreate_lock);
 if (_is_initialized) {
   return;
 }
 create_table(threads->length());
 _is_initialized = true;
   }
  
  Thanks,
  Serguei
  
  
  
  On 9/16/19 19:26, David Holmes wrote:

Hi
  Daniil,
  
  
  Thanks again for your perseverance on this one.
  
  
  I think there is a problem with initialization of the thread
  table. Suppose thread T1 has called
  ThreadsList::find_JavaThread_from_java_tid and has commenced
  execution of ThreadTable::lazy_initialize, but not yet marked
  _is_initialized as true. Now two new threads (T2 and T3) are
  created and start running - they aren't added to the ThreadTable
  yet because it isn't initialized. Now T0 also calls
  ThreadsList::find_JavaThread_from_java_tid using an updated
  ThreadsList that contains T2 and T3. It also calls
  ThreadTable::lazy_initialize. If _is_initialized is still false T0
  will attempt initialization but once it gets the lock it will see
  the table has now been initialized by T1. It will then proceed to
  update the table with its own ThreadList content - adding T2 and
  T3. That is all fine. But now suppose T0 initially sees
  _is_initialized as true, it will do nothing in lazy_initialize and
  simply return to find_JavaThread_from_java_tid. But now T2 and T3
  are missing from the ThreadTable and nothing will cause them to be
  added.
  
  
  More generally any ThreadsList that is created after the
  ThreadsList that will be used for initialization, may contain
  threads that will not be added to the table.
  
  
  Thanks,
  
  David
  
  
  On 17/09/2019 4:18 am, Daniil Titov wrote:
  
  Hello,


After investigating with Claes the impact of this change on the
performance (thanks a lot Claes for helping with it!) the
conclusion was that the impact on the thread startup time is not
a blocker for this change.


I also measured the memory footprint using Native Memory
Tracking and results showed around 40 bytes per live thread.


Please review a new version of the fix, webrev.06 [1].  Just to
remind,  webrev.05 was abandoned and webrev.06 [1] is webrev.04
[3] minus changes in src/hotspot/share/services/management.cpp
(that were factored out to a separate issue [4]) and plus a
change in ThreadsList::find_JavaThread_from_java_tid() method
(please, see below)  that addresses the problem Robbin found and
puts the code that adds a new thread to the thread table inside
Threads_lock.


src/hotspot/share/runtime/threadSMR.cpp


622   if (tobj != NULL && java_tid ==
java_lang_Thread::thread_id(tobj)) {

623 MutexLocker ml(Threads_lock);

624 // Must be inside the lock to ensure that we don't
add the thread to the table

625 // that has just passed the removal point in
ThreadsSMRSupport::remove_thread()

626 if (!thread->is_exiting()) {

627   ThreadTable::add_thread(java_tid, thread);

628   return thread;

629 }

630   }


[1] Webrev: 
https://cr.openjdk.java.net/~dtitov/8185005/webrev.06

[2] Bug: https://bugs.openjdk.java.net/browse/JDK-8185005

[3] https://cr.openjdk.java.net/~dtitov/8185005/webrev.04

[4] https://bugs.openjdk.java.net/browse/JDK-8229391


Thank you,

Daniil


  
 >

 > On 8/4/19, 7:54 PM, "David Holmes"
 wrote:

 >

 >  Hi Daniil,

 >

 >  On 3/08/2019 8:16 am, Daniil Titov wrote:

 >  > Hi David,

 >  >

 >  > Thank you for your detailed review.
Please review a new version of the fix that includes

 >  > the changes you 

Re: RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

[serguei.spit...@oracle.com]

  
  
Hi Daniil,
  
  Thank you for you patience in working on this issue!
  Also, I like that the current thread related optimizations in
  management.cpp were factored out.
  It was a good idea to separate them.
  
  I have a concern about the checks for thread->is_exiting().
The threads are added to and removed from the ThreadTable under
protection of Threads_lock.
However, the thread->is_exiting()
checks are not protected, and so, they are racy.

There is a couple of such checks to mention:
   611 JavaThread* ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) const {
 612   ThreadTable::lazy_initialize(this);
 613   JavaThread* thread = ThreadTable::find_thread_by_tid(java_tid);
 614   if (thread == NULL) {
 615 // If the thread is not found in the table find it
 616 // with a linear search and add to the table.
 617 for (uint i = 0; i < length(); i++) {
 618   thread = thread_at(i);
 619   oop tobj = thread->threadObj();
 620   // Ignore the thread if it hasn't run yet, has exited
 621   // or is starting to exit.
 622   if (tobj != NULL && java_tid == java_lang_Thread::thread_id(tobj)) {
 623 MutexLocker ml(Threads_lock);
 624 // Must be inside the lock to ensure that we don't add the thread to the table
 625 // that has just passed the removal point in ThreadsSMRSupport::remove_thread()
 626 if (!thread->is_exiting()) {
 627   ThreadTable::add_thread(java_tid, thread);
 628   return thread;
 629 }
 630   }
 631 }
 632   } else if (!thread->is_exiting()) {
 633   return thread;
 634   }
 635   return NULL;
 636 }
    ...
93 void ThreadTable::lazy_initialize(const ThreadsList *threads) {
  94   if (!_is_initialized) {
  95 {
  96   MutexLocker ml(ThreadTableCreate_lock);
  97   if (!_is_initialized) {
  98 create_table(threads->length());
  99 _is_initialized = true;
 100   }
 101 }
 102 for (uint i = 0; i < threads->length(); i++) {
 103   JavaThread* thread = threads->thread_at(i);
 104   oop tobj = thread->threadObj();
 105   if (tobj != NULL && !thread->is_exiting()) {
 106 jlong java_tid = java_lang_Thread::thread_id(tobj);
 107 add_thread(java_tid, thread);
 108   }
 109 }
 110   }
 111 }
  
A thread may start exiting right after the checks at the lines
626 and 105.
So that:
 - the lines 632-633 are useless as they do not really protect
from returning an exiting thread
 - the lines 105-108 can result in adding exiting threads into
the ThreadTable

Please, note, the lines 626-629 are safe in terms of addition to
the ThreadTable as they
are protected with the Threads_lock. But the returned thread
still can exit after that.
It is interesting what might happen if an exiting thread is
returned by the
  ThreadsList::find_JavaThread_from_java_tid
  ().

Does it make sense to develop a test that would cover these
cases?

Thanks,
Serguei

  
  On 9/16/19 11:18, Daniil Titov wrote:


  Hello,

After investigating with Claes the impact of this change on the performance (thanks a lot Claes for helping with it!) the conclusion was that the impact on the thread startup time is not a blocker for this change.

I also measured the memory footprint using Native Memory Tracking and results showed around 40 bytes per live thread.

Please review a new version of the fix, webrev.06 [1].  Just to remind,  webrev.05 was abandoned and webrev.06 [1] is webrev.04 [3] minus changes in src/hotspot/share/services/management.cpp (that were factored out to a separate issue [4]) and plus a change in ThreadsList::find_JavaThread_from_java_tid() method (please, see below)  that addresses the problem Robbin found and puts the code that adds a new thread to the thread table inside Threads_lock.

src/hotspot/share/runtime/threadSMR.cpp

622   if (tobj != NULL && java_tid == java_lang_Thread::thread_id(tobj)) {
623 MutexLocker ml(Threads_lock);
624 // Must be inside the lock to ensure that we don't add the thread to the table
625 // that has just passed the removal point in ThreadsSMRSupport::remove_thread()
626 if (!thread->is_exiting()) {
627   ThreadTable::add_thread(java_tid, thread);
628   return thread;
629 }
630   }

[1] Webrev:  https://cr.openjdk.java.net/~dtitov/8185005/webrev.06 
[2] Bug: https://bugs.openjdk.java.net/browse/JDK-8185005 
[3] https://cr.openjdk.java.net/~dtitov/8185005/webrev.04 
[4] https://bugs.openjdk.java.net/browse/JDK-8229391 

Thank you,
Daniil

 

> 
> On 8/4/19, 7:54 PM, "David Holmes"  wrote:
> 
>  Hi Daniil,
>