RFR(S): 8222295 more baseline cleanups from Async Monitor Deflation project

[Daniel D. Daugherty]

On 4/23/19 3:01 PM, coleen.phillimore at oracle.com wrote:
>
>
> On 4/23/19 12:58 PM, Daniel D. Daugherty wrote:
>> Filed the following new bug:
>>
>>     JDK-8222893 markOopDesc::print_on() is a bit confused
>> https://bugs.openjdk.java.net/browse/JDK-8222893
>>
>> Coleen, please let me know if I've captured the confusion here... :-)
>>
>> Dan
>>
>> P.S.
>> What can I say? It's code that deals with mark oops, on-stack locks,
>> biased locks and inflated locks... If there was ever code that had
>> a right to be confused... ROFL...
>>
>>
>> On 4/23/19 12:36 PM, Daniel D. Daugherty wrote:
>>> On 4/23/19 11:41 AM, coleen.phillimore at oracle.com wrote:
>>>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/4-for-jdk13.8222295/src/hotspot/share/oops/markOop.cpp.frames.html 
>>>>
>>>>
>>>>   37     if (mon == NULL) {
>>>>   38       st->print("NULL (this should never be seen!)");
>>>>   39     } else {
>>>> 40 st->print("{contentions=0x%08x,waiters=0x%08x"
>>>>   41                 ",recursions=" INTPTR_FORMAT ",owner=" 
>>>> INTPTR_FORMAT "}",
>>>> 42 mon->contentions(), mon->waiters(), mon->recursions(),
>>>>   43                 p2i(mon->owner()));
>>>>   44     }
>>>>
>>>>
>>>> Following convention, it seems like this code should be in 
>>>> ObjectMonitor::print_on(outputStream* st) so markOop doesn't have 
>>>> to know objectMonitor fields/accessors.
>>>
>>> That's a really interesting point... When you take a look at the
>>> whole of the markOopDesc::print_on() function, it is trying to
>>> give _some_ visibility into the interpretation of the various
>>> things that we have encoded into the mark oop word/header.
>>> For example, if the mark "is locked", it has this code:
>>>
>>>   45   } else if (is_locked()) {
>>>   46     st->print(" locked(" INTPTR_FORMAT ")->", value());
>>>   47     if (is_neutral()) {
>>>   48       st->print("is_neutral");
>>>   49       if (has_no_hash()) {
>>>   50         st->print(" no_hash");
>>>   51       } else {
>>>   52         st->print(" hash=" INTPTR_FORMAT, hash());
>>>   53       }
>>>   54       st->print(" age=%d", age());
>>>   55     } else if (has_bias_pattern()) {
>>>   56       st->print("is_biased");
>>>   57       JavaThread* jt = biased_locker();
>>>   58       st->print(" biased_locker=" INTPTR_FORMAT, p2i(jt));
>>>   59     } else {
>>>   60       st->print("??");
>>>   61     }
>>>
>>> and if the mark "is unlocked", it has this code:
>>>
>>>   62   } else {
>>>   63     assert(is_unlocked() || has_bias_pattern(), "just checking");
>>>   64     st->print("mark(");
>>>   65     if (has_bias_pattern()) st->print("biased,");
>>>   66     st->print("hash " INTPTR_FORMAT ",", hash());
>>>   67     st->print("age %d)", age());
>>>   68   }
>>>
>>> So I understand the reasons for the limited peek into the
>>> ObjectMonitor for the mark "has monitor" case since we do
>>> that limited level of detail for the other interpretations
>>> of the mark oop header.
>>>
>>> Summary: I'm not planning on changing that for this bug.
>>>
>>> However, now that I've pasted these code snippets, I think I
>>> see some confusion here. The mark "is locked" and mark "is unlocked"
>>> branches both have code for biased locking. That seems strange to
>>> me, but that should be looked at separately.
>>>
>
> The difference I see is that the is_locked() branches of 
> markOop::print() code don't try to print *inside* another object, like 
> ObjectLocker, which I'd like to see separated from markOop printing.  
> It can be done via. this new bug.  There are a lot of disparate things 
> in the markOop header (which should be MarkWord but that's another issue).
>
> Printing the biased locking thread didn't seem out of place here, I 
> have to admit.  If we printed fields in the Thread, that would be 
> different.

No argument about "inside" versus what's already there. What I was
trying to say was that the only way to print anything interesting
about a mark oop word that refers to an ObjectMonitor is to peek
inside that ObjectMonitor.

Dan

>
>>>
>>>> Otherwise looks like a good self-contained cleanup to me.
>>>
>>> Thanks! You'll see some of your other requested changes in the
>>> review thread for JDK-8153224 (CR1/v2.01/4-for-jdk13).
>
> Thank you for making these changes.
>
> Coleen
>
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Coleen 
>>
>