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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon May 6 18:25:43 UTC 2019 


On 5/6/19 1:46 PM, Patricio Chilano wrote:
> 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.

Okay, I'll print it in hex then.  Interpreting it as a markOop doesn't 
make that much sense, since it's not really a real markOop.
Thanks!
Coleen
>
>> 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