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

[coleen.phillimore at oracle.com]

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.

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