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

Patricio Chilano patricio.chilano.mateo at oracle.com
Mon May 6 17:46:04 UTC 2019 

Hi Coleen,

On 5/4/19 9:22 AM, coleen.phillimore at oracle.com wrote:
>
> 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?
Thanks! I would just print the header in hex though instead of trying to 
interpret it, but if you want to leave like that I'm okay too.

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


Thanks!
Patricio
> 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