RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Sep 27 17:58:23 UTC 2019
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