Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8222893

markOopDesc::print_on() is a bit confused

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: P4 P4
    • 13
    • 13
    • hotspot
    • b20
    • generic
    • generic

      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

            coleenp Coleen Phillimore
            dcubed Daniel Daugherty
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: