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

David Holmes david.holmes at oracle.com
Fri Sep 20 03:24:03 UTC 2019


On 20/09/2019 12:54 pm, Daniil Titov wrote:
> Hi David,
> 
> Thank you for reviewing this version of the fix.
> 
>>     I agree with previous comments about the general race with is_exiting in
>>     terms of how this API behaves. But there's no change in that behaviour
>>     with your changes AFAICS.
> 
> Could you please say am I right that you are referring here to the discussion about
> the fact that find_JavaThread_from_java_tid()  still could return the thread that
> is exiting? 

Yes you are right.

> If so then, yes, this behavior was not changed. We discussed it with Serguei
> and the conclusion was that since the current implementation behaves exact the same way
> then even if decide somehow address this then it makes sense to do it in a separate issue.
> It also is not clear at this moment what the possible solution could be. The obvious one,
> just to hold Thread_locks in the callers of find_JavaThread_from_java_tid()  (management.cpp)
> is too expensive and doesn't look as acceptable.

As far as I am concerned it is just an optimization to try to avoid 
returning a JavaThread that is in the process of exiting. It is an 
unavoidable race. Even holding the Threads_lock doesn't really help as 
the thread can still be "exited" for all intents and purposes even if 
the JavaThread can't truly terminate. The ThreadSMR now ensures that it 
is always safe to inspect a JavaThread. If we catch such a thread late 
in its termination sequence then its threadObj will be NULL and we'll 
just skip it; or we'll access it in a VM op and see it is_exiting, or 
some combination of such checks. So I don't see any bug here that needs 
to be fixed.

Cheers,
David

> Thanks!
> 
> Best regards,
> Daniil
> 
> 
> On 9/19/19, 6:15 PM, "David Holmes" <david.holmes at oracle.com> wrote:
> 
>      Hi Daniil,
>      
>      On 20/09/2019 10:30 am, Daniil Titov wrote:
>      > Hi David and Serguei,
>      >
>      > Please review new version of the fix that includes the changes Serguei suggested:
>      >   1. If racing threads initialize the thread table only one of these threads will populate the table with the threads from the thread list
>      
>      Ok.
>      
>      >   2. The code that adds the thread to the tread table is put inside Threads_lock to ensure that we cannot accidentally add the thread
>      >       that has just passed the removal point in ThreadsSMRSupport::remove_thread()
>      
>      That seems good too.
>      
>      I agree with previous comments about the general race with is_exiting in
>      terms of how this API behaves. But there's no change in that behaviour
>      with your changes AFAICS. The main concern, now addressed, is that you
>      mustn't be able to add a thread that never gets removed.
>      
>      Thanks,
>      David
>      -----
>      
>      > The changes are in ThreadTable::lazy_initialize() method only.
>      >
>      > Testing:  Mach5 tier1, tier2, tier3, tier4, and tier5 tests successfully passed.
>      >
>      > Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.07/
>      > Bug : https://bugs.openjdk.java.net/browse/JDK-8185005
>      >
>      > Thank you!
>      > --Daniil
>      >
>      > On 9/18/19, 1:01 AM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:
>      >
>      >      Hi Daniil,
>      >
>      >      On 9/17/19 17:13, Daniil Titov wrote:
>      >      > 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.
>      >
>      >      Now, I'm not that confident about 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.
>      >
>      >      If I understand it correctly, the jt->threadObj() can remain non-NULL
>      >      for some time while jt->is_exiting() == true.
>      >      It is not clear how reliable is to use it.
>      >      But this is a pre-existing issue. It is not you who introduced it. :)
>      >
>      >      So, we can skip it for now.
>      >      But for the record, we may have a source of intermittent issues.
>      >
>      >      > 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.
>      >
>      >      Understand.
>      >
>      >      > 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()
>      >
>      >
>      >      Okay, thanks!
>      >
>      >      > 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.
>      >
>      >
>      >      I've replied to David in another email.
>      >      Let's talk once more about it tomorrow.
>      >
>      >
>      >      > 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.
>      >
>      >      Yes. Please, see above.
>      >
>      >      Thanks,
>      >      Serguei
>      >
>      >      > Best regards,
>      >      > Daniil
>      >      >
>      >      > From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>      >      > Date: Tuesday, September 17, 2019 at 1:53 AM
>      >      > To: Daniil Titov <daniil.x.titov at oracle.com>, Robbin Ehn <robbin.ehn at oracle.com>, David Holmes <david.holmes at oracle.com>, <daniel.daugherty at oracle.com>, OpenJDK Serviceability <serviceability-dev at openjdk.java.net>, "hotspot-runtime-dev at openjdk.java.net" <hotspot-runtime-dev at openjdk.java.net>, "jmx-dev at openjdk.java.net" <jmx-dev at openjdk.java.net>, Claes Redestad <claes.redestad at oracle.com>
>      >      > 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()) {
>      >      >   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" mailto:david.holmes at 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" mailto:david.holmes at 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" mailto:daniel.daugherty at 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: mailto:serguei.spitsyn at oracle.com
>      >      >          >      >      >      > Organization: Oracle Corporation
>      >      >          >      >      >      > Date: Friday, June 28, 2019 at 7:56 PM
>      >      >          >      >      >      > To: Daniil Titov mailto:daniil.x.titov at oracle.com, OpenJDK Serviceability mailto:serviceability-dev at openjdk.java.net, mailto:hotspot-runtime-dev at openjdk.java.net mailto:hotspot-runtime-dev at openjdk.java.net, mailto:jmx-dev at openjdk.java.net mailto:jmx-dev at 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" mailto:david.holmes at 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
>      >      >          >      >      >      >
>      >      >          >      >      >      >
>      >      >          >      >      >      >
>      >      >          >      >      >      >
>      >      >          >      >      >      >
>      >      >          >      >      >      >
>      >      >          >      >      >      >
>      >      >          >      >      >
>      >      >          >      >      >
>      >      >          >      >      >
>      >      >          >      >      >
>      >      >          >      >
>      >      >          >      >
>      >      >          >      >
>      >      >          >
>      >      >          >
>      >      >          >
>      >      >
>      >      >
>      >      >
>      >      >
>      >      >
>      >      >
>      >      >
>      >      >
>      >      >
>      >      >
>      >      >
>      >
>      >
>      >
>      >
>      
> 
> 


More information about the hotspot-runtime-dev mailing list