8273107: RunThese24H times out with "java.lang.management.ThreadInfo.getLockName()" is null #25
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -279,6 +279,18 @@ void VM_ThreadDump::doit() { | ||
concurrent_locks.dump_at_safepoint(); | ||
} | ||
|
||
ObjectMonitorsHashtable table; | ||
ObjectMonitorsHashtable* tablep = nullptr; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like you can remove this pointer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now From this call site, when For other |
||
if (_with_locked_monitors) { | ||
// The caller wants locked monitor information and that's expensive to gather | ||
// when there are a lot of inflated monitors. So we deflate idle monitors and | ||
// gather information about owned monitors at the same time. | ||
tablep = &table; | ||
while (ObjectSynchronizer::deflate_idle_monitors(tablep) >= (size_t)MonitorDeflationMax) { | ||
; /* empty */ | ||
} | ||
} | ||
|
||
if (_num_threads == 0) { | ||
// Snapshot all live threads | ||
|
||
@@ -293,7 +305,7 @@ void VM_ThreadDump::doit() { | ||
if (_with_locked_synchronizers) { | ||
tcl = concurrent_locks.thread_concurrent_locks(jt); | ||
} | ||
snapshot_thread(jt, tcl); | ||
snapshot_thread(jt, tcl, tablep); | ||
} | ||
} else { | ||
// Snapshot threads in the given _threads array | ||
@@ -328,14 +340,15 @@ void VM_ThreadDump::doit() { | ||
if (_with_locked_synchronizers) { | ||
tcl = concurrent_locks.thread_concurrent_locks(jt); | ||
} | ||
snapshot_thread(jt, tcl); | ||
snapshot_thread(jt, tcl, tablep); | ||
} | ||
} | ||
} | ||
|
||
void VM_ThreadDump::snapshot_thread(JavaThread* java_thread, ThreadConcurrentLocks* tcl) { | ||
void VM_ThreadDump::snapshot_thread(JavaThread* java_thread, ThreadConcurrentLocks* tcl, | ||
ObjectMonitorsHashtable* table) { | ||
ThreadSnapshot* snapshot = _result->add_thread_snapshot(java_thread); | ||
snapshot->dump_stack_at_safepoint(_max_depth, _with_locked_monitors); | ||
snapshot->dump_stack_at_safepoint(_max_depth, _with_locked_monitors, table); | ||
snapshot->set_concurrent_locks(tcl); | ||
} | ||
|
||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -659,7 +659,7 @@ ThreadStackTrace::~ThreadStackTrace() { | ||
} | ||
} | ||
|
||
void ThreadStackTrace::dump_stack_at_safepoint(int maxDepth) { | ||
void ThreadStackTrace::dump_stack_at_safepoint(int maxDepth, ObjectMonitorsHashtable* table) { | ||
assert(SafepointSynchronize::is_at_safepoint(), "all threads are stopped"); | ||
|
||
if (_thread->has_last_Java_frame()) { | ||
@@ -683,9 +683,19 @@ void ThreadStackTrace::dump_stack_at_safepoint(int maxDepth) { | ||
|
||
if (_with_locked_monitors) { | ||
// Iterate inflated monitors and find monitors locked by this thread | ||
// not found in the stack | ||
// that are not found in the stack, e.g. JNI locked monitors: | ||
InflatedMonitorsClosure imc(this); | ||
ObjectSynchronizer::monitors_iterate(&imc, _thread); | ||
if (table != nullptr) { | ||
// Get the ObjectMonitors locked by the target thread, if any, | ||
// and does not include any where owner is set to a stack lock | ||
// address in the target thread: | ||
ObjectMonitorsHashtable::PtrList* list = table->get_entry(_thread); | ||
if (list != nullptr) { | ||
ObjectSynchronizer::monitors_iterate(&imc, list, _thread); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The InflatedMonitorsClosure walks the stack until object is found with this method: If you instead just collect all locks on stack with one pass you don't have to walk the stack over and over, which should be major speedup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like an interesting RFE, but I'd prefer that be investigated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clarification: ThreadStackTrace::is_owned_monitor_on_stack() is not actually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sorry, we loop the stack frame info for every object. I.e. a loop in a loop which is bad for time-complexity. |
||
} | ||
} else { | ||
ObjectSynchronizer::monitors_iterate(&imc, _thread); | ||
} | ||
} | ||
} | ||
|
||
@@ -936,9 +946,10 @@ ThreadSnapshot::~ThreadSnapshot() { | ||
delete _concurrent_locks; | ||
} | ||
|
||
void ThreadSnapshot::dump_stack_at_safepoint(int max_depth, bool with_locked_monitors) { | ||
void ThreadSnapshot::dump_stack_at_safepoint(int max_depth, bool with_locked_monitors, | ||
ObjectMonitorsHashtable* table) { | ||
_stack_trace = new ThreadStackTrace(_thread, with_locked_monitors); | ||
_stack_trace->dump_stack_at_safepoint(max_depth); | ||
_stack_trace->dump_stack_at_safepoint(max_depth, table); | ||
} | ||
|
||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm. I'm not sure this is correct. The owned of an ObjectMonitor is a JavaThread most of the time, but not always. I think that if the owner started off with a stack lock, and another thread inflated the lock due to contention, then the ObjectMonitor will get the stack lock as the owner, as the random other thread is too lazy to look up which thread stack the stack lock belongs to.
Because of that, I think we might miss locks that really are owned by a thread, as the thread key now will not map to such locks. Some kind of ownership lookup probably needs to happen here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch on an oddity of ObjectMonitor ownership!
The baseline code has the same "issue":
void ObjectSynchronizer::monitors_iterate(MonitorClosure* closure, JavaThread* thread) {
<snip>
Please notice that I said "issue" and not "bug". This isn't a bug and you really
won't like the reason that it's not a bug. I know that I don't like it...
Because of this ownership check
monitors_iterate()
won't apply the closure(InflatedMonitorsClosure in this case) to an ObjectMonitor that is marked as
owned by a stack lock address because it will never match the target JavaThread*.
Here's the closure:
This particular monitors_iterate() call uses InflatedMonitorsClosure to
gather just the JNI locked monitors owned by the target JavaThread*.
A JNI locked monitor will never have a stack lock address as its owner
marking. It will always have a JavaThread*. So the baseline code isn't
"broken" because it will find the JNI locked monitors just fine. Similarly
the new code isn't "broken" because it will also find the JNI locked
monitors just fine.
I definitely need to clarify the baseline comments and the new code
comments to make this subtlety much more clear.