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

[Daniel D. Daugherty]

Coleen,

Thanks for the quick review!

More below...

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.


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

Dan


>
> Coleen
>
> On 4/23/19 10:28 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have a (S)mall patch extracted from the Async Monitor Deflation 
>> project
>> that is ready for code review.
>>
>> Karen, a number of the changes here are from your code review comments
>> to the parent bug:
>>
>>     JDK-8153224 Monitor deflation prolong safepoints
>>     https://bugs.openjdk.java.net/browse/JDK-8153224
>>
>> The short version of what this patch is about:
>>
>>     More baseline cleanups to the ObjectMonitor subsystem.
>>
>> The details are in the bug report:
>>
>>     JDK-8222295 more baseline cleanups from Async Monitor Deflation 
>> project
>>     https://bugs.openjdk.java.net/browse/JDK-8222295
>>
>> Here's the webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/4-for-jdk13.8222295/
>>
>> This patch along with the current patch for Async Monitor Deflation
>> project have been through Mach5 tier[1-8] testing.
>>
>> I have been actively using the revised assert()'s and guarantee()'s with
>> additional diagnostic info while debugging my port of the Async Monitor
>> Deflation project code.
>>
>> Thanks, in advance, for any questions, comments or suggestions.
>>
>> Dan
>