RFR 8150689: Thread dump report "waiting to re-lock in wait()" shows incorrectly

[David Holmes]

On 22/11/2018 8:41 am, Patricio Chilano wrote:
> Hi David,
> 
> On 11/20/18 5:34 PM, David Holmes wrote:
>> Hi Patricio,
>>
>> On 21/11/2018 3:51 am, Patricio Chilano wrote:
>>> Hi all,
>>>
>>> Could you review this small fix for thread dump reports?
>>> Now the "waiting to re-lock in wait()" message is shown in the "at 
>>> java.lang.Object.wait(Native Method)" frame where the re-locking is 
>>> actually occurring. In the frame where the lock was first taken we 
>>> always show "locked". Please check if there is some scenario where 
>>> you think the lock info on the stack is not being printed as you 
>>> would expect.
>>
>> Can you please update the bug report with examples of how the stack 
>> report now looks compared to those used to discuss the bug.
> 
> Ok, I updated the bug showing the stack report for the example discussed 
> there and I added a new one showing when "waiting to re-lock in wait()" 
> will now be displayed.
> 
>> I applied the patch and ran my examples and they looked good!
>>
>>> I also noticed the _thread_state attribute was printed twice so I 
>>> removed one instance. I don't know if there was a specific reason to 
>>> print it twice so I can add it back.
>>
>> ThreadSafepointState::print_on is called from both the stack dump code 
>> (where you see _thread_state twice) and safepoint logging. It's useful 
>> to also see the thread state during safepoint logging so please undo 
>> this change.
> 
> Ok, but wouldn't removing "print_thread_state_on(st);" line before 
> "_safepoint_state->print_on(st);" on method JavaThread::print_on be okay 
> then? I actually moved up the print of thread state in file 
> safepoint.cpp to kept printing the states together as we do now.

Doh! Yes you are right - sorry.

Thanks,
David

> 
>> src/hotspot/share/runtime/vframe.cpp
>>
>> 182 if(java_lang_Thread::get_thread_status(thread()->threadObj()) == 
>> java_lang_Thread::BLOCKED_ON_MONITOR_ENTER){
>>
>> Nit: need space after "if", need space after ")"
>> Nit please line-break after == and indent appropriately
> 
> Done!
> 
>> This seems such an obvious check I have to really question why we did 
>> not do this before, and had the comment that the two states could not 
>> be distinguished. :)
>>
>> I think what you have done simplifies things nicely, and as I said my 
>> two main examples now work fine.
> 
> Thanks! Yes, I spent some time trying to figure out that comment.
> 
>>> Webrev URL: http://cr.openjdk.java.net/~pchilanomate/8150689.01/webrev
>>> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8150689
>>
>> Please always advise what testing you did. In this case I think we 
>> have a problem with a test, but it seems to be a pre-existing problem:
>>
>> serviceability/tmtools/jstack/WaitNotifyThreadTest.java
>>
>> Seems to expect to find a "waiting to re-lock" line but in fact I 
>> can't see anything in the test that actually creates a situation where 
>> the thread would be waiting to re-lock! Further the test is supposed 
>> to print out:
>>
>> System.out.println("Correct monitor info found");
>>
>> yet there is no such output. This indicates that the test is not 
>> working as expected and is not actually finding any locks. I think the 
>> logic for checking the method "name" is broken e.g.
>>
>>      if (mi.getName().startsWith("WaitThread.run")) {
>>
>> the actual name is WaitNotifyThreadTest$WaitThread.run so it won't match!
>>
>> Also:
>>
>> if (mi.getName().startsWith(OBJECT_WAIT) && mi.getCompilationUnit() == 
>> null /*native method*/) {
>>
>> won't match anything either as far as I can see!
>>
>> <sigh> I filed a bug for that test: JDK-8214148
> 
> Yes, sorry forgot to do that. I actually run mach5 tiers1-2 and saw no 
> errors. I see this test executes as part of tier2 so it should have 
> failed. I'll look into that. Thanks for filing the bug!
> 
> 
> Waiting for the duplicate removal confirmation to make the new webrev 
> (Dan also made some comments on that).
> 
> 
> Thanks!
> Patricio
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Patricio
>