JDK-8222893 : markOopDesc::print_on() is a bit confused
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 13
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2019-04-23
  • Updated: 2020-05-14
  • Resolved: 2019-05-06
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 13
13 b20Fixed
Related Reports
Relates :  
Relates :  
Relates :  
Relates :  
Description
During the code review for:

    JDK-8222295 more baseline cleanups from Async Monitor Deflation project

Some comments came up about this function:

src/hotspot/share/oops/markOop.cpp: markOopDesc::print_on()

Here's the email thread:

On 4/23/19 11:41 AM, coleen.phillimore@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 
Comments
Another email... On 4/23/19 3:07 PM, coleen.phillimore@oracle.com wrote: > >>>> On 4/23/19 12:36 PM, Daniel D. Daugherty wrote: >>>>> On 4/23/19 11:41 AM, coleen.phillimore@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
23-04-2019

Yes, the code inside the "(is_locked())" branch seems to be in the wrong place. That code should be actually on the last branch where the object is unlocked. The "is_locked()" branch will be executed when the last two bits of the markword are 0x00, since the "is_locked()" checks for a value != 0x01, and the 0x11 and 0x10 cases were already checked for. That's either a thin lock or inflation. The "is_neutral()" case and the "has_bias_pattern()" will be true for the last branch where the last two bits of the markword are 0x01.
23-04-2019

Added [~coleenp] and [~pchilanomate] to this bug. Coleen because it was her code review comment and Patricio because there's something weird with the biased locking code in markOopDesc::print_on().
23-04-2019