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

[Daniel D. Daugherty]

On 1/14/19 8:26 AM, David Holmes wrote:
> On 14/01/2019 11:21 pm, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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.
>
> It may not be very helpful but it is more helpful than nothing.
>
> But I guess I can remove it.

Just for the record, I would recommend keeping it. I find it helpful
to have assertions in places where the code has reached something
unexpected. Instead of:

   assert(false, "OBJECT_WAIT state is unexpected");

I would do it as:

   fatal("OBJECT_WAIT state is unexpected");

to catch it in any form of bits (product, debug, etc).

As for tracking back a future occurrence to this review, I often
check to see what changeset added an assertion to see the context
in which the assertion was placed.

The fix is already pushed so again this is just for the record.

Dan



>
> Thanks,
> David
> -----
>
>>
>> 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
>>>>
>>