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

[Daniel D. Daugherty]

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...


>
>>
>>     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...


>>
>>     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
>>
>