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

[David Holmes]

On 17/01/2019 6:57 am, coleen.phillimore at oracle.com wrote:
> On 1/16/19 11:52 AM, Daniel D. Daugherty wrote:
>> 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).
> 
> No, this is even worse!  

I have to agree with Coleen here - at most an assert is warranted so 
that during testing if we hit such an unexpected condition it is brought 
to our attention. But if the unexpected condition hits in the wild we do 
not want to crash the VM! Nothing bad happens if we find the unexpected 
condition, we just won't report anything.

For the record it was pushed with no assert, as Coleen requested.

David

> Do you really want some rarely reproduceable 
> crash coming from a customer?  The assertion isn't even checking for 
> some error in processing, it's for a helpful diagnostic.  The only 
> reason for guarantees is that something *very* bad will happen later.
> 
> Anyway, this is somewhat of a minor point whether there should have been 
> an assert or not.  My recommendation was what I already wrote.
> thanks,
> Coleen
>>
>> 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
>>>>>>
>>>>
>>
>