[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2019-09-27 18:08:17
Message-ID: 35780c4b-1399-f585-cc41-b4c06a93c45d () oracle ! com
[Download RAW message or body]
On 9/27/19 11:06, Daniel D. Daugherty wrote:
> On 9/27/19 1:58 PM, 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?
>
> Pushed on 2019.09.25 at 1416 ET. It has made it thru Tier7 testing
> as of yesterday...
Nice.
Thanks, Dan!
Serguei
>
> 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@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