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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Sep 27 18:06:12 UTC 2019


On 9/27/19 1:58 PM, serguei.spitsyn at oracle.com wrote:
> Hi Daniil,
>
> Just notice I did not reply to you.
> Thank you for the explanation!
>
> Have you already pushed this one?

Pushed on 2019.09.25 at 1416 ET. It has made it thru Tier7 testing
as of yesterday...

Dan


>
> Thanks,
> Serguei
>
>
> On 9/24/19 12:46, Daniil Titov wrote:
>> Hi Serguei,
>>
>> Thank you for reviewing this version of the fix.
>>
>>>     Just one question about ThreadIdTable::remove_thread(jlong tid).
>>>     What happens if there is no thread with the specified tid in 
>>> ThreadIdTable?
>>>     Is it possible?
>> It could be possible when the thread that was started while the 
>> thread table
>> was initializing exits.  At this point the thread table is 
>> initialized and the thread
>> tries to remove itself from it. Removing non-existing  entry from 
>> ConcurrentHashTable
>> is a correct operation that just leaves the table unchanged.
>>
>> src/hotspot/share/services/threadIdTable.cpp
>>
>>     233    bool ThreadIdTable::remove_thread(jlong tid) {
>>     234      assert(_is_initialized, "Thread table is not initialized");
>>     235      Thread* thread = Thread::current();
>>     236      ThreadIdTableLookup lookup(tid);
>>     237      return _local_table->remove(thread, lookup);
>>     238    }
>>
>> src/hotspot/share/utilities/concurrentHashTable.hpp
>>
>>    422      // Returns true if items was deleted matching LOOKUP_FUNC 
>> and
>>     423      // prior to destruction DELETE_FUNC is called.
>>     424      template <typename LOOKUP_FUNC, typename DELETE_FUNC>
>>     425      bool remove(Thread* thread, LOOKUP_FUNC& lookup_f, 
>> DELETE_FUNC& del_f) {
>>     426        return internal_remove(thread, lookup_f, del_f);
>>     427      }
>>     428
>>     429      // Same without DELETE_FUNC.
>>     430      template <typename LOOKUP_FUNC>
>>     431      bool remove(Thread* thread, LOOKUP_FUNC& lookup_f) {
>>     432        return internal_remove(thread, lookup_f, noOp);
>>     433      }
>>
>> src/hotspot/share/utilities/concurrentHashTable.inline.hpp
>>
>>     446    inline bool ConcurrentHashTable<CONFIG, F>::
>>     447      internal_remove(Thread* thread, LOOKUP_FUNC& lookup_f, 
>> DELETE_FUNC& delete_f)
>>     448    {
>>     449      Bucket* bucket = get_bucket_locked(thread, 
>> lookup_f.get_hash());
>>     450      assert(bucket->is_locked(), "Must be locked.");
>>     451      Node* const volatile * rem_n_prev = bucket->first_ptr();
>>     452      Node* rem_n = bucket->first();
>>     453      bool have_dead = false;
>>     454      while (rem_n != NULL) {
>>     455        if (lookup_f.equals(rem_n->value(), &have_dead)) {
>>     456          bucket->release_assign_node_ptr(rem_n_prev, 
>> rem_n->next());
>>     457          break;
>>     458        } else {
>>     459          rem_n_prev = rem_n->next_ptr();
>>     460          rem_n = rem_n->next();
>>     461        }
>>     462      }
>>     463
>>     464      bucket->unlock();
>>     465
>>     466      if (rem_n == NULL) {
>>     467        return false;
>>     468      }
>>
>> Best regards,
>> Daniil
>>
>>
>> On 9/24/19, 11:35 AM, "serguei.spitsyn at oracle.com" 
>> <serguei.spitsyn at oracle.com> wrote:
>>
>>      Hi Daniil,
>>           This version looks good to me.
>>      Thank you for the update!
>>           Just one question about ThreadIdTable::remove_thread(jlong 
>> tid).
>>      What happens if there is no thread with the specified tid in 
>> ThreadIdTable?
>>      Is it possible?
>>           Thanks,
>>      Serguei
>>           On 9/24/19 9:36 AM, Daniil Titov wrote:
>>      > Hi Daniel, David and Serguei,
>>      >
>>      > Please review a new version of the fix (webrev.08) that as 
>> Daniel suggested renames
>>      > ThreadTable to ThreadIdTable (related classes and variables 
>> are renamed as well) and
>>      > corrects formatting issues. There are no other changes in this 
>> webrev.08 comparing
>>      > to the previous version webrev.07.
>>      >
>>      > Testing: Mach5 tier1, tier2, tier3, tier4, and tier5 tests 
>> successfully passed.
>>      >
>>      > Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.08/
>>      > Bug: : https://bugs.openjdk.java.net/browse/JDK-8185005
>>      >
>>      > Thank you!
>>      >
>>      > Best regards,
>>      > Daniil
>>      >
>>      > On 9/20/19, 2:59 PM, "Daniel D. Daugherty" 
>> <daniel.daugherty at oracle.com> wrote:
>>      >
>>      >      Daniil,
>>      >
>>      >      Thanks for sticking with this project through the many 
>> versions.
>>      >      Sorry this review is late...
>>      >
>>      >
>>      >      On 9/19/19 8:30 PM, 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
>>      >      >   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()
>>      >      >
>>      >      > 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/
>>      >
>>      >      src/hotspot/share/runtime/mutexLocker.hpp
>>      >           No comments.
>>      >
>>      >      src/hotspot/share/runtime/mutexLocker.cpp
>>      >           No comments.
>>      >
>>      >      src/hotspot/share/runtime/threadSMR.cpp
>>      >           L623:         MutexLocker ml(Threads_lock);
>>      >           L626:         if (!thread->is_exiting()) {
>>      >               Re: discussion about is_exiting()
>>      >
>>      >               The header comment is pretty clear:
>>      >
>>      >                 src/hotspot/share/runtime/thread.hpp:
>>      >
>>      >                   // thread has called JavaThread::exit() or 
>> is terminated
>>      >                   bool is_exiting() const;
>>      >
>>      >               is_exiting() might become true right after you 
>> have called it,
>>      >               but its purpose is to ask the question and not 
>> prevent the
>>      >               condition from becoming true. As David said, you 
>> should consider
>>      >               it an optimization. If you happen to see the 
>> condition is true,
>>      >               then you know that the JavaThread isn't going to 
>> be around much
>>      >               longer and should act accordingly.
>>      >
>>      >               The is_exiting() implementation is:
>>      >
>>      >                 inline bool JavaThread::is_exiting() const {
>>      >                   // Use load-acquire so that setting of 
>> _terminated by
>>      >                   // JavaThread::exit() is seen more quickly.
>>      >                   TerminatedTypes l_terminated = 
>> (TerminatedTypes)
>>      > OrderAccess::load_acquire((volatile jint *) &_terminated);
>>      >                   return l_terminated == _thread_exiting ||
>>      >      check_is_terminated(l_terminated);
>>      >                 }
>>      >
>>      >               and it depends on the JavaThread's _terminated 
>> field value.
>>      >
>>      >                 // JavaThread termination support
>>      >                 enum TerminatedTypes {
>>      >                  _not_terminated = 0xDEAD - 2,
>>      > _thread_exiting,                             //
>>      >      JavaThread::exit() has been called for this thread
>>      > _thread_terminated,                          // JavaThread
>>      >      is removed from thread list
>>      > _vm_exited                                   // JavaThread
>>      >      is still executing native code, but VM is terminated
>> > // only VM_Exit
>>      >      can set _vm_exited
>>      >                 };
>>      >
>>      >               so the JavaThread's _terminated field can get 
>> set to
>>      >               _thread_exiting independent of the Threads_lock, 
>> but
>>      >               it can't get set to _thread_terminated without the
>>      >               Threads_lock.
>>      >
>>      >               So by grabbing the Threads_lock on L623, you 
>> make sure
>>      >               that ThreadTable::add_thread(java_tid, thread) 
>> does not
>>      >               add a JavaThread that's not on the ThreadsList. 
>> It might
>>      >               still become is_exiting() == true right after your
>>      >
>>      >                 L626         if (!thread->is_exiting()) {
>>      >
>>      >               but it will still be on the main ThreadsList. 
>> And that
>>      >               means that when the JavaThread is removed from 
>> the main
>>      >               ThreadsList, you'll still call:
>>      >
>>      >                 L931: ThreadTable::remove_thread(tid);
>>      >
>>      >           L624:         // Must be inside the lock to ensure 
>> that we don't
>>      >      add the thread to the table
>>      >               typo: s/the thread/a thread/
>>      >
>>      >           L633:       return thread;
>>      >               nit - L633 - indented too far (should be 2 spaces)
>>      >
>>      >      src/hotspot/share/services/threadTable.hpp
>>      >           L42:   static void lazy_initialize(const ThreadsList 
>> *threads);
>>      >               nit - put space between '*' the variable:
>>      >
>>      >                 static void lazy_initialize(const ThreadsList* 
>> threads);
>>      >
>>      >               like you do in your other decls.
>>      >
>>      >           L45:   // Lookup and inserts
>>      >               Perhaps:  // Lookup and list management
>>      >
>>      >           L60-61 - nit - please delete these blank lines.
>>      >
>>      >      src/hotspot/share/services/threadTable.cpp
>>      >           L28: #include "runtime/timerTrace.hpp"
>>      >               nit - This should be after threadSMR.hpp... 
>> (alpha sorted order)
>>      >
>>      >           L39: static const size_t DefaultThreadTableSizeLog = 8;
>>      >               nit - your other 'static const' are not 
>> CamelCase. Why is this one?
>>      >
>>      >           L45: static ThreadTableHash* volatile _local_table = 
>> NULL;
>>      >           L50: static volatile size_t _current_size = 0;
>>      >           L51: static volatile size_t _items_count = 0;
>>      >               nit - can you group the file statics together? 
>> (up with L41).
>>      >
>>      >           L60:     _tid(tid),_java_thread(java_thread) {}
>>      >               nit - space after ','
>>      >
>>      >           L62   jlong tid() const { return _tid;}
>>      >           L63   JavaThread* thread() const {return _java_thread;}
>>      >               nit - space before '}'
>>      >               nit - space after '{' on L63.
>>      >
>>      >           L70:     static uintx get_hash(Value const& value, 
>> bool* is_dead) {
>>      >               Parameter 'is_dead' is not used.
>>      >
>>      >           L74:     static void* allocate_node(size_t size, 
>> Value const& value) {
>>      >               Parameter 'value' is not used.
>>      >
>>      >           L93: void ThreadTable::lazy_initialize(const 
>> ThreadsList *threads) {
>>      >               Re: discussion about lazy_initialize() racing with
>>      > ThreadsList::find_JavaThread_from_java_tid()
>>      >
>>      >               There's a couple of aspects to these two pieces 
>> of code racing
>>      >               with each other and racing with new thread 
>> creation. Racing with
>>      >               new thread creation is the easy one:
>>      >
>>      >                 If a new thread isn't added to the ThreadTable by
>>      >                 ThreadsSMRSupport::add_thread() calling
>>      >      ThreadTable::add_thread(),
>>      >                 then the point in the future where someone calls
>>      >                 find_JavaThread_from_java_tid() will add it to 
>> the table due to
>>      >                 the linear search when 
>> ThreadTable::find_thread_by_tid()
>>      >                 returns NULL.
>>      >
>>      >              As for multi-threads calling
>>      >      ThreadsList::find_JavaThread_from_java_tid()
>>      >              at the same time which results in multi-threads 
>> in lazy_initialize()
>>      >              at the same time...
>>      >
>>      >              - ThreadTable creation will be linear due to 
>> ThreadTableCreate_lock.
>>      >                After _is_initialized is set to true, then no 
>> more callers to
>>      >                lazy_initialize() will be in the "if 
>> (!_is_initialized)" block.
>>      >              - Once the ThreadTable is created, then 
>> multi-threads can be
>>      >                executing the for-loop to add their ThreadsList 
>> entries to
>>      >                the ThreadTable. There will be a bit of 
>> Threads_lock contention
>>      >                as each of the multi-threads tries to add their 
>> entries and
>>      >                there will be some wasted work since the 
>> multi-threads will
>>      >                likely have similar ThreadLists.
>>      >
>>      >              Of course, once _is_initialized is set to true, 
>> then any caller
>>      >              to lazy_initialize() will return quickly and
>>      > ThreadsList::find_JavaThread_from_java_tid() will call
>>      >              ThreadTable::find_thread_by_tid(). If the target 
>> java_tid isn't
>>      >              found, then we do the linear search thing here 
>> and add the
>>      >              the entry if we find a match in our current 
>> ThreadsList. Since
>>      >              we're only adding the one here, we only contend 
>> for the Threads_lock
>>      >              here if we find it.
>>      >
>>      >              If ThreadsList::find_JavaThread_from_java_tid() 
>> is called with a
>>      >              target java_tid for a JavaThread that was created 
>> after the
>>      >              ThreadsList object that the caller has in hand 
>> for the
>>      >              find_JavaThread_from_java_tid() call, then, of 
>> course, that
>>      >              target 'java_tid' won't be found because the 
>> JavaThread was
>>      >              added the main ThreadsList _after_ the 
>> ThreadsList object was
>>      >              created by the caller. Of course, you have to ask 
>> where the
>>      >              target java_tid value came from since the 
>> JavaThread wasn't
>>      >              around when the 
>> ThreadsList::find_JavaThread_from_java_tid()
>>      >              call was made with that target java_tid value...
>>      >
>>      >           L99:         // being concurently populated during 
>> the initalization.
>>      >               Typos? Perhaps:
>>      >                        // to be concurrently populated during 
>> initialization.
>>      >
>>      >               But I think those two comment lines are more 
>> appropriate above
>>      >               this line:
>>      >
>>      >               L96:       MutexLocker ml(ThreadTableCreate_lock);
>>      >
>>      >           L112:           // Must be inside the lock to ensure 
>> that we don't
>>      >      add the thread to the table
>>      >               typo: s/the thread/a thread/
>>      >
>>      >           L141:   return ((double)_items_count)/_current_size;
>>      >               nit - need spaces around '/'.
>>      >
>>      >           L177:   bool equals(ThreadTableEntry **value, bool* 
>> is_dead) {
>>      >               nit - put space between '**' the variable:
>>      >                   bool equals(ThreadTableEntry** value,
>>      >
>>      >               Parameter 'is_dead' is not used.
>>      >
>>      >           L214:   while(true) {
>>      >               nit - space before '('.
>>      >
>>      >
>>      >      Short version: Thumbs up.
>>      >
>>      >      Longer version: I don't think I've spotted anything other 
>> than nits here.
>>      >      Mostly I've just looked for multi-threaded races, proper 
>> usage of the
>>      >      Thread-SMR stuff, and minimal impact in the case where 
>> the new
>>      >      ThreadsTable is never needed.
>>      >
>>      >      Dan
>>      >
>>      >      P.S.
>>      >      ThreadTable is a bit of misnomer. What you really have 
>> here is
>>      >      a ThreadIdTable, but I'm really late to the code review flow
>>      >      with that comment...
>>      >
>>      >
>>      >      > Bug : https://bugs.openjdk.java.net/browse/JDK-8185005
>>      >      >
>>      >      > Thank you!
>>      >      > --Daniil
>>      >
>>      >
>>      >
>>      >
>>
>>
>
>



More information about the hotspot-runtime-dev mailing list