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

Daniil Titov daniil.x.titov at oracle.com
Fri Sep 27 18:07:51 UTC 2019


Hi Serguei,

Yes, it  is already pushed in the repository [1].

[1] https://hg.openjdk.java.net/jdk/jdk/rev/f4abe950c3b0

Best regards,
Daniil

On 9/27/19, 10:58 AM, "serguei.spitsyn at oracle.com" <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?
    
    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