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

[coleen.phillimore at oracle.com]

On 4/23/19 3:04 PM, Daniel D. Daugherty wrote:
> 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.

So this is fine as is, if you want to make an 
ObjectMonitor::print_on(outputStream*st) and call it in this separate bug.

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