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 serviceability-dev
mailing list