RFR (S): 8213397: Stack dump should show more clearly when a thread is blocked on a class initialization monitor

[coleen.phillimore at oracle.com]

On 1/14/19 8:13 AM, David Holmes wrote:
> Hi Coleen,
>
> On 14/01/2019 10:25 pm, coleen.phillimore at oracle.com wrote:
>>
>> This is a nice improvement.
>
> Thanks for taking a look.
>
>> http://cr.openjdk.java.net/~dholmes/8213397/webrev/src/hotspot/share/runtime/vframe.cpp.udiff.html 
>>
>>
>> + // top-frame, so we should be waiting on a Class initialization 
>> monitor.
>> + InstanceKlass* k = thread()->class_to_be_initialized();
>> + if (k != NULL) {
>> + st->print_cr("\t- waiting on the Class initialization monitor for 
>> %s", k->external_name());
>> + }
>> + else {
>> + assert(false, "OBJECT_WAIT state is unexpected");
>> + }
>>
>>
>> Should be } else {
>>
>> And I don't think this should be an assert.  There are a couple of 
>> other places that we lock an object monitor from the VM, but I don't 
>> think we wait in these places.  In any case, if we add a wait, it 
>> would take a lot of testing and a long time to get to this assert to 
>> find it needs to add another case here.  So if you remove the 'else' 
>> completely and change the comment to "see if we are waiting on a 
>> class initialization monitor" you don't need to fix the {}.
>
> Initially I thought the "else" might be some JNI or JVM TI case, but 
> that's not so as there's no direct API to do wait(), it just requires 
> invoking the Object.wait method. So at the moment the implicit else is 
> "unreachable" and I wanted to convey that. I chose the assert in case 
> I was wrong, or in the unlikely event a new VM internal call to wait() 
> was ever added and we happened to encounter it. Leaving it empty to me 
> raises the question "how do we get here?".

The thing is, that leaving an assert to find some future case is only 
good if that code is going to execute frequently and we find it in 
testing.  This is not a frequent path.   Having the assert might be hit 
much later down the road when we've forgotten this.  It's not helpful.

Thanks,
Coleen

>
> Thanks,
> David
>
>> Thanks,
>> Coleen
>>
>>
>>
>> On 1/14/19 1:01 AM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213397
>>> webrev: http://cr.openjdk.java.net/~dholmes/8213397/webrev/
>>>
>>> Please review this simple enhancement to improve the stack trace 
>>> information when a thread is blocked doing a (logical) Object.wait 
>>> on the monitor associated with Class static initialization.
>>>
>>> Details are in the bug report but basically we record, in a 
>>> thread-local variable, the name of the class before the "wait" and 
>>> clear it after, thus allowing the stack dump code to query it if it 
>>> appears we may be in that situation.
>>>
>>> In InstanceKlass::initialize_impl I also refactored the code a tiny 
>>> bit to make use of the JavaThread reference to the current thread.
>>>
>>> A new test using jstack was also added.
>>>
>>> Testing:
>>>   - mach5 tiers 1 - 3
>>>   - the new test on Windows/Linux/OSX x86 and Solaris sparc, in 
>>> regular and Xcomp mode
>>>   - All jstack/stack-dump using tests I could see (linux x64):
>>>     - runtime/Thread
>>>     - serviceability/sa/
>>>     - serviceability/tmtools/jstack/
>>>     - vmTestbase/nsk/jvmti/scenarios/sampling/
>>>     - vmTestbase/nsk/jvmti/MonitorWaited/ .
>>>     - vmTestbase/nsk/jvmti/GetOwnedMonitorInfo/
>>>
>>> Thanks,
>>> David
>>