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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Sat May 4 13:22:04 UTC 2019 

Patricio, Thank you for all your help writing the test!

On 5/3/19 7:01 PM, Patricio Chilano wrote:
> 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.

I can add printing out the prototype_header in the Klass like:

java.lang.Byte
{0x0000000715e110f0} mark(is_biased biased_locker=0x0000000000a4c800 
epoch=0 age 0)
  - prototype_header: mark(is_biased biased_locker=0x0000000000000000 
epoch=0 age 0)
  - klass: public final synchronized 'java/lang/Byte'
  - ---- fields (total size 2 words):
  - private final 'value' 'B' @12  0

The biased_locker field isn't iteresting though, but you can tell by the 
epoch and biased bit in the prototype_header that the object is really 
biased or not.  These lines line up outside of email.  Would this be useful?

 From you comment below, I changed the type of the object to be 
Byte_klass because it's unlikely for code to lock these.

http://cr.openjdk.java.net/~coleenp/2019/8222893.02/webrev/index.html

Thanks,
Coleen
>
> 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
>>>>>
>>>>
>>>
>>
>

More information about the hotspot-runtime-dev mailing list