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

[coleen.phillimore at oracle.com]

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

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

Thanks!
Coleen

>
> Thumbs up.
>
> Dan
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8222893
>>
>> Thanks,
>> Coleen
>