RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Sep 20 21:59:09 UTC 2019
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