RFR (S) 8222893: markOopDesc::print_on() is a bit confused

[coleen.phillimore at oracle.com]

On 5/3/19 2:44 PM, Daniel D. Daugherty wrote:
> On 5/3/19 2:18 PM, coleen.phillimore at oracle.com wrote:
>> Dan, thank you for reviewing!
>>
>> On 5/3/19 12:08 PM, Daniel D. Daugherty wrote:
>>> Thanks for taking care of this bug.
>>>
>>>
>>> On 5/3/19 11:31 AM, coleen.phillimore at oracle.com wrote:
>>>> Summary: Add print_on for ObjectMonitor and make markOop printing 
>>>> sensible and add test.
>>>>
>>>> Testing with mach5 ongoing, but tested locally.
>>>>
>>>> open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2019/8222893.01/webrev
>>>
>>> src/hotspot/share/oops/klass.cpp
>>>     old L737:   ResourceMark rm;
>>>         Why delete the ResourceMark?
>>
>> The print functions with outputStream should not have a ResourceMark 
>> because you could call it with a LogStream or stream that was 
>> allocated outside the resource mark. The caller needs the 
>> ResourceMark.   This was a bug my test found.
>
> Thanks filling in the details.
>
>
>>>
>>>     L744:      st->cr();
>>>         As long as there are no tests that depend on this 'WizardMode'
>>>         output style, this should be okay. (Please search the tests
>>>         for use of WizardMode'.
>>
>> There's only one test in compilercontrol that had WizardMode so I ran 
>> those.
>>>
>>> src/hotspot/share/oops/markOop.cpp
>>>     L47:     if (is_neutral()) {   // 001 biased bit in 3rd right bit
>>>         Previously your comment was '// last bits = ???'
>>>         I think that was a better style.
>>>
>>>     L54:     } else if (has_bias_pattern()) {  // 101
>>>         Previously your comment was '// last bits = ???'
>>>         I think that was a better style.
>>
>> I see.   I changed it like this:
>>
>>   } else {
>>     st->print(" mark(");
>>     // Biased bit is 3rd rightmost bit
>>     if (is_neutral()) {   // last bits = 001
>> ...
>>     } else if (has_bias_pattern()) {  // last bits = 101
>>
>> On those lines.  I wanted to note that the biased bit is the third one.
>>>
>>> src/hotspot/share/runtime/objectMonitor.cpp
>>>     No comments.
>>>
>>> src/hotspot/share/runtime/objectMonitor.hpp
>>>     No comments.
>>>
>>> test/hotspot/gtest/oops/test_markOop.cpp
>>>     L67:     // Notify gets the lock inflated
>>>         Actually it is wait() that gets the lock inflated.
>>
>> I'll fix the comment to "Wait gets the lock inflated."  But the lock 
>> stays inflated after the notify, doesn't it?  Because that's what I'm 
>> testing for:
>>
>>     // Wait gets the lock inflated.
>>     ObjectLocker ol(h_obj, THREAD);
>>     ol.notify_all(THREAD);
>>     assert_test_pattern(h_obj, "monitor");  // lock stays inflated
>
> The object will stay locked for the context of 'ol' so the lock will
> still be inflated after the notify_all() call. Deflation can't happen
> while an ObjectMonitor is "busy" and being locked is the most "busy"
> state we have...
>

:) I added this commentary to the test.
>
>>
>>>
>>>     L88:   assert_test_pattern(h_obj, "is_biased");
>>>         A comment about why a newly created object would be biased
>>>         would help here. Something like:
>>>
>>>         // Biased locking is enabled for java.lang.Object:
>>
>> Added without the colon.  I set the UseBiasedLocking flag explicitly 
>> to be immune to external flag setting (and reset).
>
> Thinking about it... the following is more correct:
>
>     // java.lang.Object can be bias locked initially.
>
> As in there haven't been enough bias revocations to cause a 
> java.lang.Object
> to be marked as not bias-able. Patricio may have a better idea for the
> wording...
>

I don't know if it can be revoked at this stage in the test.  I guess 
that assume initialization doesn't do a lot of locking for different 
threads for java.lang.Object.  It seems unlikely.

Speaking of Patricio, he helped me write the test.
thanks,
Coleen
>
>>>
>>>     There are some gtest pieces here that I'm not familiar with,
>>>     but the test looks good to me. Obviously someone with more
>>>     gtest experience should also review.
>>
>> I used some constructs that Robbin added for the concurrent hashtable 
>> tests.
>
> Cool.
>
> Dan
>
>
>>
>> Thanks!
>> Coleen
>>
>>>
>>> Thumbs up.
>>>
>>> Dan
>>>
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8222893
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>
>