RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
David Holmes
david.holmes at oracle.com
Tue Sep 24 22:45:10 UTC 2019
Looks good to me.
Thanks,
David
On 25/09/2019 2: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