[jdk18] RFR: 8273107: RunThese24H times out with "java.lang.management.ThreadInfo.getLockName()" is null [v3]

[Daniel D.Daugherty]

On Fri, 17 Dec 2021 15:52:31 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   coleenp CR - cleanup type safety
>
> For the record, Threads::owning_thread_from_monitor_owner seems to be the function to look up who *really* own an ObjectMonitor, based on the value in the owner field, to deal with the stuff I talked about.

I need to address these comments from @fisk:

> When the owner field of an ObjectMonitor is the address of a stack lock instead
> of a thread, it doesn't mean that it is a stack lock. It means that it was a stack
> lock when it was acquired by that thread, but has since then been upgraded to
> a full ObjectMonitor (by another thread), due to contention. But the fact that it
> was a stack lock is irrelevant in this context; we care about what it is now. And
> it is an inflated lock that just happens to have a slightly obscured owner field,
> as the owner thread was not easily available when it was inflated.

What @fisk says here is correct and I'm probably guilty of over simplifying what
I was trying to explain earlier. Here's what I said earlier for context:

>> The baseline code has the same "issue":
>>
>> void ObjectSynchronizer::monitors_iterate(MonitorClosure* closure, JavaThread* thread) {
>>
>> <snip>
>>
>>     ObjectMonitor* mid = iter.next();
>>     if (mid->owner() != thread) {
>>       continue;
>>     }
>> 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*.

@fisk, please notice that I don't say that this is a stack lock. I say this is

>> an ObjectMonitor that is marked as owned by a stack lock address

and that is the "bug" in your opinion. The baseline code and new code
do not recognize that the owner is not a JavaThread* and don't apply
the closure even though the stack lock address might belong to the
current target JavaThread.

I think we are in strong agreement about what the attributes of the "bug" are.


> In other words, I still believe this is a bug. Such a monitor that gets upgraded
> from a stack lock to a full ObjectMonitor, can - unline actual stack locks - be
> involved in a deadlock that we are trying to find. If we don't associate such
> ObjectMonitor objects with its owner thread, then we will not be able to find
> the deadlock as we are supposed to with this code.

You are missing part of the algorithm in the code that calls monitors_iterate().
The monitors_iterate() part ONLY EXISTS to find the JNI locked ObjectMonitors.
It does that by finding all of the ObjectMonitors on the in-use list that are owned
by the target JavaThread* (and by owned I mean _owner == JavaThread*). It uses
the closure to search for each matched ObjectMonitor* in the JavaThread's stack
info that it squirreled away before making the call to monitors_iterate(). If it finds
the ObjectMonitor* on the JavaThread's stack, then that ObjectMonitor* IS NOT
a JNI locked ObjectMonitor and monitors_iterate() ignores it.

If monitors_iterate() does not find the ObjectMonitor* in the JavaThread's stack
info, then it assumes that the ObjectMonitor* is a JNI locked monitor and it adds
that ObjectMonitor* to the thread dump info.

Because an ObjectMonitor that is marked as owned by a stack lock address won't
match the target JavaThread*, it won't get processed by the closure that is called
by monitors_iterate(). However, we don't care because that ObjectMonitor* IS NOT
a JNI locked monitor and that is what monitors_iterate() is looking for. It must be a
monitor that is mentioned in the JavaThread's stack because it was acquired that
way originally so it will ALREADY be included in the thread's info that was gathered
before we made the monitors_iterate() call. Nothing is missed here!

I have tried to clarify this in the comments that I added as part of this patch. I'll
re-review the comments again and see if there is anything else I can add or change.

-------------

PR: https://git.openjdk.java.net/jdk18/pull/25