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

[Patricio Chilano]

Hi Coleen,

Change looks good to me. I was going to suggest to try to output 
different descriptions for the case where a lock has the bias pattern 
and a current valid biaser, against the case where the lock is 
"biasable". But to do that we would also need to read the prototype 
header of the klass, and we can't access it from markOop.cpp : ( . The 
reason to try to differentiate those cases is because I think the term 
"object is biased" should apply when there is a current valid biaser, 
otherwise the output should be "object has bias pattern" for the general 
case.

More below on your discussion with Dan...

On 5/3/19 3:34 PM, coleen.phillimore at oracle.com wrote:
>
> 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 think both comments sound okay. Maybe a combination like "Biased 
locking is initially enabled for java.lang.Object"

> 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.
Right, unless after executing BiasedLocking::init() the initialization 
code executes too many revocations for objects of class java.lang.Object 
such that a bulk revocation takes place, the newly created object should 
have a bias pattern (Maybe it's better to pick a less common class?). If 
you want to be sure you can read the prototype header of the class first 
and check if it has the bias pattern before trying to exercise bias locking.

Thanks!
Patricio

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