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" <david.hol...@oracle.com> 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" <david.hol...@oracle.com> 
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 suggested:
     >          >      > - ThreadTableCreate_lock scope is reduced to cover the 
creation of the table only;
     >          >      > - ThreadTableCreate_lock is made 
_safepoint_check_always;
     >          >
     >          >      Okay.
     >          >
     >          >      > - ServiceThread is no longer responsible for the 
resizing of the thread table, instead,
     >          >      >    the thread table is changed to grow on demand by 
the thread that is doing the addition;
     >          >
     >          >      Okay - I'm happy to get the serviceThread out of the 
picture here.
     >          >
     >          >      > - fixed nits and formatting issues.
     >          >
     >          >      Okay.
     >          >
     >          >      >>> The change also includes additional optimization for 
some callers of find_JavaThread_from_java_tid()
     >          >      >>>   as Daniel suggested.
     >          >      >> Not sure it's best to combine these, but if they are 
limited to the
     >          >      >> changes in management.cpp only then that may be okay.
     >          >      >
     >          >      > The additional optimization for some callers of 
find_JavaThread_from_java_tid() is
     >          >      > limited to management.cpp (plus a new test) so I left 
them in the webrev  but
     >          >      > I also could move it in the separate issue if required.
     >          >
     >          >      I'd prefer this part of be separated out, but won't 
insist. Let's see if
     >          >      Dan or Serguei have a strong opinion.
     >          >
     >          >      >    > src/hotspot/share/runtime/threadSMR.cpp
     >          >      >    >755     jlong tid = 
SharedRuntime::get_java_tid(thread);
     >          >      >    > 926     jlong tid = 
SharedRuntime::get_java_tid(thread);
     >          >      >   >  I think it cleaner/better to just use
     >          >      >   > jlong tid = 
java_lang_Thread::thread_id(thread->threadObj());
     >          >      >   > as we know thread is not NULL, it is a JavaThread 
and it has to have a
     >          >      >   > non-null threadObj.
     >          >      >
     >          >      > I had to leave this code unchanged since it turned out 
the threadObj is null
     >          >      > when VM is destroyed:
     >          >      >
     >          >      > V  [libjvm.so+0xe165d7]  oopDesc::long_field(int) 
const+0x67
     >          >      > V  [libjvm.so+0x16e06c6]  
ThreadsSMRSupport::add_thread(JavaThread*)+0x116
     >          >      > V  [libjvm.so+0x16d1302]  Threads::add(JavaThread*, 
bool)+0x82
     >          >      > V  [libjvm.so+0xef8369]  
attach_current_thread.part.197+0xc9
     >          >      > V  [libjvm.so+0xec136c]  jni_DestroyJavaVM+0x6c
     >          >      > C  [libjli.so+0x4333]  JavaMain+0x2c3
     >          >      > C  [libjli.so+0x8159]  ThreadJavaMain+0x9
     >          >
     >          >      This is actually nothing to do with the VM being 
destroyed, but is an
     >          >      issue with JNI_AttachCurrentThread and its interaction 
with the
     >          >      ThreadSMR iterators. The attach process is:
     >          >      - create JavaThread
     >          >      - mark as "is attaching via jni"
     >          >      - add to ThreadsList
     >          >      - create java.lang.Thread object (you can only execute 
Java code after
     >          >      you are attached)
     >          >      - mark as "attach completed"
     >          >
     >          >      So while a thread "is attaching" it will be seen by the 
ThreadSMR thread
     >          >      iterator but will have a NULL java.lang.Thread object.
     >          >
     >          >      We special-case attaching threads in a number of places 
in the VM and I
     >          >      think we should be explicitly doing something here to 
filter out
     >          >      attaching threads, rather than just being tolerant of a 
NULL j.l.Thread
     >          >      object. Specifically in ThreadsSMRSupport::add_thread:
     >          >
     >          >      if (ThreadTable::is_initialized() && 
!thread->is_attaching_via_jni()) {
     >          >         jlong tid = 
java_lang_Thread::thread_id(thread->threadObj());
     >          >         ThreadTable::add_thread(tid, thread);
     >          >      }
     >          >
     >          >      Note that in ThreadsSMRSupport::remove_thread we can use 
the same guard,
     >          >      which covers the case the JNI attach encountered an 
error trying to
     >          >      create the j.l.Thread object.
     >          >
     >          >      >> src/hotspot/share/services/threadTable.cpp
     >          >      >> 71     static uintx get_hash(Value const& value, 
bool* is_dead) {
     >          >      >
     >          >      >> The is_dead parameter still bothers me here. I can't 
make enough sense
     >          >      >> out of the template code in ConcurrentHashtable to 
see why we have to
     >          >      >> have it, but I'm concerned that its very existence 
means we perhaps
     >          >      >> should not be trying to extend CHT in this context. ??
     >          >      >
     >          >      > My understanding is that is_dead parameter provides a 
mechanism for
     >          >      > ConcurrentHashtable to remove stale entries that were 
not explicitly
     >          >      > removed by calling  ConcurrentHashTable::remove() 
method.
     >          >      > I think that just because in our case we don't use 
this mechanism doesn't
     >          >      > mean we should not use ConcurrentHashTable.
     >          >
     >          >      Can you confirm that this usage is okay with Robbin Ehn 
please. He's
     >          >      back from vacation this week.
     >          >
     >          >      >> I would still want to see what impact this has on 
thread
     >          >      >> startup cost, both with and without the table being 
initialized.
     >          >      >
     >          >      > I run a test that initializes the table by calling 
ThreadMXBean.get getThreadInfo(),
     >          >      > starts some threads as a worm-up, and then creates and 
starts 100,000 threads
     >          >      > (each thread just sleeps for 100 ms). In case when the 
thread table is enabled
     >          >      > 100,000 threads are created and started  for about 
15200 ms. If the thread table
     >          >      > is off the test takes about 14800 ms. Based on this 
information the enabled
     >          >      > thread table makes the thread startup about 2.7% 
slower.
     >          >
     >          >      That doesn't sound very good. I think we may need to 
Claes involved to
     >          >      help investigate overall performance impact here.
     >          >
     >          >      > Webrev: 
https://cr.openjdk.java.net/~dtitov/8185005/webrev.04/
     >          >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
     >          >
     >          >      No further code comments.
     >          >
     >          >      I didn't look at the test in detail.
     >          >
     >          >      Thanks,
     >          >      David
     >          >
     >          >      > Thanks!
     >          >      > --Daniil
     >          >      >
     >          >      >
     >          >      > On 7/29/19, 12:53 AM, "David Holmes" 
<david.hol...@oracle.com> wrote:
     >          >      >
     >          >      >      Hi Daniil,
     >          >      >
     >          >      >      Overall I think this is a reasonable approach but 
I would still like to
     >          >      >      see some performance and footprint numbers, both 
to verify it fixes the
     >          >      >      problem reported, and that we are not getting 
penalized elsewhere.
     >          >      >
     >          >      >      On 25/07/2019 3:21 am, Daniil Titov wrote:
     >          >      >      > Hi David, Daniel, and Serguei,
     >          >      >      >
     >          >      >      > Please review the new version of the fix, that 
makes the thread table initialization on demand and
     >          >      >      > moves it inside 
ThreadsList::find_JavaThread_from_java_tid(). At the creation time the thread table
     >          >      >      >   is initialized with the threads from the 
current thread list. We don't want to hold Threads_lock
     >          >      >      > inside find_JavaThread_from_java_tid(),  thus 
new threads still could be created  while the thread
     >          >      >      > table is being initialized . Such threads will 
be found by the linear search and added to the thread table
     >          >      >      > later, in 
ThreadsList::find_JavaThread_from_java_tid().
     >          >      >
     >          >      >      The initialization allows the created but 
unpopulated, or partially
     >          >      >      populated, table to be seen by other threads - is 
that your intention?
     >          >      >      It seems it should be okay as the other threads 
will then race with the
     >          >      >      initializing thread to add specific entries, and 
this is a concurrent
     >          >      >      map so that should be functionally correct. But 
if so then I think you
     >          >      >      can also reduce the scope of the 
ThreadTableCreate_lock so that it
     >          >      >      covers creation of the table only, not the 
initial population of the table.
     >          >      >
     >          >      >      I like the approach of only initializing the 
table when needed and using
     >          >      >      that to control when the add/remove-thread code 
needs to update the
     >          >      >      table. But I would still want to see what impact 
this has on thread
     >          >      >      startup cost, both with and without the table 
being initialized.
     >          >      >
     >          >      >      > The change also includes additional 
optimization for some callers of find_JavaThread_from_java_tid()
     >          >      >      > as Daniel suggested.
     >          >      >
     >          >      >      Not sure it's best to combine these, but if they 
are limited to the
     >          >      >      changes in management.cpp only then that may be 
okay. It helps to be
     >          >      >      able to focus on the table related changes 
without being distracted by
     >          >      >      other optimizations.
     >          >      >
     >          >      >      > That is correct that ResolvedMethodTable was 
used as a blueprint for the thread table, however, I tried
     >          >      >      > to strip it of the all functionality that is 
not required in the thread table case.
     >          >      >
     >          >      >      The revised version seems better in that regard. 
But I still have a
     >          >      >      concern, see below.
     >          >      >
     >          >      >      > We need to have the thread table resizable and 
allow it to grow as the number of threads increases to avoid
     >          >      >      > reserving excessive memory a-priori or 
deteriorating lookup times. The ServiceThread is responsible for
     >          >      >      > growing the thread table when required.
     >          >      >
     >          >      >      Yes but why? Why can't this table be grown on 
demand by the thread that
     >          >      >      is doing the addition? For other tables we may 
have to delegate to the
     >          >      >      service thread because the current thread cannot 
perform the action, or
     >          >      >      it doesn't want to perform it at the time the 
need for the resize is
     >          >      >      detected (e.g. its detected at a safepoint and 
you want the resize to
     >          >      >      happen later outside the safepoint). It's not 
apparent to me that such
     >          >      >      restrictions apply here.
     >          >      >
     >          >      >      > There is no ConcurrentHashTable available in 
Java 8 and for backporting this fix to Java 8 another implementation
     >          >      >      > of the hash table, probably originally 
suggested in the patch attached to the JBS issue, should be used.  It will make
     >          >      >      > the backporting more complicated,  however, 
adding a new Implementation of the hash table in Java 14 while it
     >          >      >      > already has ConcurrentHashTable doesn't seem  
reasonable for me.
     >          >      >
     >          >      >      Ok.
     >          >      >
     >          >      >      > Webrev: 
http://cr.openjdk.java.net/~dtitov/8185005/webrev.03
     >          >      >
     >          >      >      Some specific code comments:
     >          >      >
     >          >      >      src/hotspot/share/runtime/mutexLocker.cpp
     >          >      >
     >          >      >      +   def(ThreadTableCreate_lock       , 
PaddedMutex  , special,
     >          >      >      false, Monitor::_safepoint_check_never);
     >          >      >
     >          >      >      I think this needs to be a 
_safepoint_check_always lock. The table will
     >          >      >      be created by regular JavaThreads and they should 
(nearly) always be
     >          >      >      checking for safepoints if they are going to 
block acquiring the lock.
     >          >      >      And it isn't at all obvious that the thread doing 
the creation can't go
     >          >      >      to a safepoint whilst this lock is held.
     >          >      >
     >          >      >      ---
     >          >      >
     >          >      >      src/hotspot/share/runtime/threadSMR.cpp
     >          >      >
     >          >      >      Nit:
     >          >      >
     >          >      >        618       JavaThread* thread = thread_at(i);
     >          >      >
     >          >      >      you could reuse the new java_thread local you 
introduced at line 613 and
     >          >      >      just rename that "new" variable to "thread" so 
you don't have to change
     >          >      >      all other uses.
     >          >      >
     >          >      >      628   } else if (java_thread != NULL && ...
     >          >      >
     >          >      >      You don't need to check != NULL here as you only 
get here when
     >          >      >      java_thread is not NULL.
     >          >      >
     >          >      >        755     jlong tid = 
SharedRuntime::get_java_tid(thread);
     >          >      >        926     jlong tid = 
SharedRuntime::get_java_tid(thread);
     >          >      >
     >          >      >      I think it cleaner/better to just use
     >          >      >
     >          >      >      jlong tid = 
java_lang_Thread::thread_id(thread->threadObj());
     >          >      >
     >          >      >      as we know thread is not NULL, it is a JavaThread 
and it has to have a
     >          >      >      non-null threadObj.
     >          >      >
     >          >      >      ---
     >          >      >
     >          >      >      src/hotspot/share/services/management.cpp
     >          >      >
     >          >      >      1323         if (THREAD->is_Java_thread()) {
     >          >      >      1324           JavaThread* current_thread = 
(JavaThread*)THREAD;
     >          >      >
     >          >      >      These calls can only be made on a JavaThread so 
this be simplified to
     >          >      >      remove the is_Java_thread() call. Similarly in 
other places.
     >          >      >
     >          >      >      ---
     >          >      >
     >          >      >      src/hotspot/share/services/threadTable.cpp
     >          >      >
     >          >      >         55 class ThreadTableEntry : public 
CHeapObj<mtInternal> {
     >          >      >         56   private:
     >          >      >         57     jlong _tid;
     >          >      >
     >          >      >      I believe hotspot style is to not indent the 
access modifiers in C++
     >          >      >      class declarations, so the above would just be:
     >          >      >
     >          >      >         55 class ThreadTableEntry : public 
CHeapObj<mtInternal> {
     >          >      >         56 private:
     >          >      >         57   jlong _tid;
     >          >      >
     >          >      >      etc.
     >          >      >
     >          >      >        60     ThreadTableEntry(jlong tid, JavaThread* 
java_thread) :
     >          >      >        61     _tid(tid),_java_thread(java_thread) {}
     >          >      >
     >          >      >      line 61 should be indented as it continues line 
60.
     >          >      >
     >          >      >         67 class ThreadTableConfig : public AllStatic {
     >          >      >         ...
     >          >      >         71     static uintx get_hash(Value const& 
value, bool* is_dead) {
     >          >      >
     >          >      >      The is_dead parameter still bothers me here. I 
can't make enough sense
     >          >      >      out of the template code in ConcurrentHashtable 
to see why we have to
     >          >      >      have it, but I'm concerned that its very 
existence means we perhaps
     >          >      >      should not be trying to extend CHT in this 
context. ??
     >          >      >
     >          >      >        115   size_t start_size_log = size_log > 
DefaultThreadTableSizeLog
     >          >      >        116   ? size_log : DefaultThreadTableSizeLog;
     >          >      >
     >          >      >      line 116 should be indented, though in this case 
I think a better layout
     >          >      >      would be:
     >          >      >
     >          >      >        115   size_t start_size_log =
     >          >      >        116       size_log > DefaultThreadTableSizeLog 
? size_log :
     >          >      >      DefaultThreadTableSizeLog;
     >          >      >
     >          >      >        131 double ThreadTable::get_load_factor() {
     >          >      >        132   return (double)_items_count/_current_size;
     >          >      >        133 }
     >          >      >
     >          >      >      Not sure that is doing what you want/expect. It 
will perform integer
     >          >      >      division and then cast that whole integer to a 
double. If you want
     >          >      >      double arithmetic you need:
     >          >      >
     >          >      >      return ((double)_items_count)/_current_size;
     >          >      >
     >          >      >      180     jlong          _tid;
     >          >      >      181     uintx         _hash;
     >          >      >
     >          >      >      Nit: no need for all those spaces before the 
variable name.
     >          >      >
     >          >      >        183     ThreadTableLookup(jlong tid)
     >          >      >        184     : _tid(tid), _hash(primitive_hash(tid)) 
{}
     >          >      >
     >          >      >      line 184 should be indented.
     >          >      >
     >          >      >      201     ThreadGet():_return(NULL) {}
     >          >      >
     >          >      >      Nit: need space after :
     >          >      >
     >          >      >        211    assert(_is_initialized, "Thread table is not 
initialized");
     >          >      >        212   _has_work = false;
     >          >      >
     >          >      >      line 211 is indented one space too far.
     >          >      >
     >          >      >      229     ThreadTableEntry* entry = new 
ThreadTableEntry(tid,java_thread);
     >          >      >
     >          >      >      Nit: need space after ,
     >          >      >
     >          >      >      252   return _local_table->remove(thread,lookup);
     >          >      >
     >          >      >      Nit: need space after ,
     >          >      >
     >          >      >      Thanks,
     >          >      >      David
     >          >      >      ------
     >          >      >
     >          >      >      > Bug: 
https://bugs.openjdk.java.net/browse/JDK-8185005
     >          >      >      >
     >          >      >      > Thanks!
     >          >      >      > --Daniil
     >          >      >      >
     >          >      >      >
     >          >      >      > On 7/8/19, 3:24 PM, "Daniel D. Daugherty" 
<daniel.daughe...@oracle.com> wrote:
     >          >      >      >
     >          >      >      >      On 6/29/19 12:06 PM, Daniil Titov wrote:
     >          >      >      >      > Hi Serguei and David,
     >          >      >      >      >
     >          >      >      >      > Serguei is right, 
ThreadTable::find_thread(java_tid) cannot  return a JavaThread with an unmatched java_tid.
     >          >      >      >      >
     >          >      >      >      > Please find a new version of the fix 
that includes the changes Serguei suggested.
     >          >      >      >      >
     >          >      >      >      > Regarding the concern about the 
maintaining the thread table when it may never even be queried, one of
     >          >      >      >      > the options could be to add ThreadTable 
::isEnabled flag, set it to "false" by default, and wrap the calls to the thread table
     >          >      >      >      > in ThreadsSMRSupport add_thread() and 
remove_thread() methods to check this flag.
     >          >      >      >      >
     >          >      >      >      > When 
ThreadsList::find_JavaThread_from_java_tid() is called for the first time it could check if 
ThreadTable ::isEnabled
     >          >      >      >      > Is on and if not then set it on and 
populate the thread table with all existing threads from the thread list.
     >          >      >      >
     >          >      >      >      I have the same concerns as David H. about 
this new ThreadTable.
     >          >      >      >      
ThreadsList::find_JavaThread_from_java_tid() is only called from code
     >          >      >      >      in 
src/hotspot/share/services/management.cpp so I think that table
     >          >      >      >      needs to enabled and populated only if it 
is going to be used.
     >          >      >      >
     >          >      >      >      I've taken a look at the webrev below and 
I see that David has
     >          >      >      >      followed up with additional comments. 
Before I do a crawl through
     >          >      >      >      code review for this, I would like to see 
the ThreadTable stuff
     >          >      >      >      made optional and David's other comments 
addressed.
     >          >      >      >
     >          >      >      >      Another possible optimization is for 
callers of
     >          >      >      >      find_JavaThread_from_java_tid() to save 
the calling thread's
     >          >      >      >      tid value before they loop and if the 
current tid == saved_tid
     >          >      >      >      then use the current JavaThread* instead 
of calling
     >          >      >      >      find_JavaThread_from_java_tid() to get the 
JavaThread*.
     >          >      >      >
     >          >      >      >      Dan
     >          >      >      >
     >          >      >      >      >
     >          >      >      >      > Webrev: 
https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/
     >          >      >      >      > Bug: 
https://bugs.openjdk.java.net/browse/JDK-8185005
     >          >      >      >      >
     >          >      >      >      > Thanks!
     >          >      >      >      > --Daniil
     >          >      >      >      >
     >          >      >      >      > From: <serguei.spit...@oracle.com>
     >          >      >      >      > Organization: Oracle Corporation
     >          >      >      >      > Date: Friday, June 28, 2019 at 7:56 PM
     >          >      >      >      > To: Daniil Titov <daniil.x.ti...@oracle.com>, OpenJDK Serviceability 
<serviceability-dev@openjdk.java.net>, "hotspot-runtime-...@openjdk.java.net" 
<hotspot-runtime-...@openjdk.java.net>, "jmx-...@openjdk.java.net" <jmx-...@openjdk.java.net>
     >          >      >      >      > Subject: Re: RFR: 8185005: Improve 
performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
     >          >      >      >      >
     >          >      >      >      > Hi Daniil,
     >          >      >      >      >
     >          >      >      >      > I have several quick comments.
     >          >      >      >      >
     >          >      >      >      > The indent in the hotspot c/c++ files 
has to be 2, not 4.
     >          >      >      >      >
     >          >      >      >      > 
https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/src/hotspot/share/runtime/threadSMR.cpp.frames.html
     >          >      >      >      > 614 JavaThread* 
ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) const {
     >          >      >      >      >   615     JavaThread* java_thread = 
ThreadTable::find_thread(java_tid);
     >          >      >      >      >   616     if (java_thread == NULL && 
java_tid == PMIMORDIAL_JAVA_TID) {
     >          >      >      >      >   617         // 
ThreadsSMRSupport::add_thread() is not called for the primordial
     >          >      >      >      >   618         // thread. Thus, we find 
this thread with a linear search and add it
     >          >      >      >      >   619         // to the thread table.
     >          >      >      >      >   620         for (uint i = 0; i < 
length(); i++) {
     >          >      >      >      >   621             JavaThread* thread = 
thread_at(i);
     >          >      >      >      >   622             if 
(is_valid_java_thread(java_tid,thread)) {
     >          >      >      >      >   623                 
ThreadTable::add_thread(java_tid, thread);
     >          >      >      >      >   624                 return thread;
     >          >      >      >      >   625             }
     >          >      >      >      >   626         }
     >          >      >      >      >   627     } else if (java_thread != NULL 
&& is_valid_java_thread(java_tid, java_thread)) {
     >          >      >      >      >   628         return java_thread;
     >          >      >      >      >   629     }
     >          >      >      >      >   630     return NULL;
     >          >      >      >      >   631 }
     >          >      >      >      >   632 bool 
ThreadsList::is_valid_java_thread(jlong java_tid, JavaThread* java_thread) {
     >          >      >      >      >   633     oop tobj = 
java_thread->threadObj();
     >          >      >      >      >   634     // Ignore the thread if it 
hasn't run yet, has exited
     >          >      >      >      >   635     // or is starting to exit.
     >          >      >      >      >   636     return (tobj != NULL && 
!java_thread->is_exiting() &&
     >          >      >      >      >   637             java_tid == 
java_lang_Thread::thread_id(tobj));
     >          >      >      >      >   638 }
     >          >      >      >      >
     >          >      >      >      >   615     JavaThread* java_thread = 
ThreadTable::find_thread(java_tid);
     >          >      >      >      >
     >          >      >      >      >    I'd suggest to rename find_thread() 
to find_thread_by_tid().
     >          >      >      >      >
     >          >      >      >      > A space is missed after the comma:
     >          >      >      >      >    622 if 
(is_valid_java_thread(java_tid,thread)) {
     >          >      >      >      >
     >          >      >      >      > An empty line is needed before L632.
     >          >      >      >      >
     >          >      >      >      > The name 'is_valid_java_thread' looks 
wrong (or confusing) to me.
     >          >      >      >      > Something like 
'is_alive_java_thread_with_tid()' would be better.
     >          >      >      >      > It'd better to list parameters in the 
opposite order.
     >          >      >      >      >
     >          >      >      >      > The call to is_valid_java_thread() is 
confusing:
     >          >      >      >      >     627 } else if (java_thread != NULL 
&& is_valid_java_thread(java_tid, java_thread)) {
     >          >      >      >      >
     >          >      >      >      > Why would the call 
ThreadTable::find_thread(java_tid) return a JavaThread with an unmatched java_tid?
     >          >      >      >      >
     >          >      >      >      >
     >          >      >      >      > Thanks,
     >          >      >      >      > Serguei
     >          >      >      >      >
     >          >      >      >      > On 6/28/19, 9:40 PM, "David Holmes" 
<david.hol...@oracle.com> wrote:
     >          >      >      >      >
     >          >      >      >      >      Hi Daniil,
     >          >      >      >      >
     >          >      >      >      >      The definition and use of this 
hashtable (yet another hashtable
     >          >      >      >      >      implementation!) will need careful 
examination. We have to be concerned
     >          >      >      >      >      about the cost of maintaining it 
when it may never even be queried. You
     >          >      >      >      >      would need to look at footprint 
cost and performance impact.
     >          >      >      >      >
     >          >      >      >      >      Unfortunately I'm just about to 
board a plane and will be out for the
     >          >      >      >      >      next few days. I will try to look 
at this asap next week, but we will
     >          >      >      >      >      need a lot more data on it.
     >          >      >      >      >
     >          >      >      >      >      Thanks,
     >          >      >      >      >      David
     >          >      >      >      >
     >          >      >      >      > On 6/28/19 3:31 PM, Daniil Titov wrote:
     >          >      >      >      > Please review the change that improves 
performance of ThreadMXBean MXBean methods returning the
     >          >      >      >      > information for specific threads. The 
change introduces the thread table that uses ConcurrentHashTable
     >          >      >      >      > to store one-to-one the mapping between 
the thread ids and JavaThread objects and replaces the linear
     >          >      >      >      > search over the thread list in 
ThreadsList::find_JavaThread_from_java_tid(jlong tid) method with the lookup
     >          >      >      >      > in the thread table.
     >          >      >      >      >
     >          >      >      >      > Testing: Mach5 tier1,tier2 and tier3 
tests successfully passed.
     >          >      >      >      >
     >          >      >      >      > Webrev: 
https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/
     >          >      >      >      > Bug: 
https://bugs.openjdk.java.net/browse/JDK-8185005
     >          >      >      >      >
     >          >      >      >      > Thanks!
     >          >      >      >      >
     >          >      >      >      > Best regards,
     >          >      >      >      > Daniil
     >          >      >      >      >
     >          >      >      >      >
     >          >      >      >      >
     >          >      >      >      >
     >          >      >      >      >
     >          >      >      >      >
     >          >      >      >      >
     >          >      >      >
     >          >      >      >
     >          >      >      >
     >          >      >      >
     >          >      >
     >          >      >
     >          >      >
     >          >
     >          >
     >          >
     >
     >
     >
     >
     >
     >
     >


Reply via email to