[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
From: Daniil Titov <daniil.x.titov () oracle ! com>
Date: 2019-09-27 18:07:51
Message-ID: 7DB8CB3A-33CF-4C1D-8C7F-0324FB269A3E () oracle ! com
[Download RAW message or body]
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@oracle.com" <serguei.spitsyn@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@oracle.com" \
<serguei.spitsyn@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@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
> >
> >
> >
> >
>
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic