-
Bug
-
Resolution: Fixed
-
P4
-
13
-
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 forJDK-8153224 (CR1/v2.01/4-for-jdk13).
Dan
>
> Coleen
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
Dan
>
> Coleen
- relates to
-
JDK-8244999 Clean up usage of ResourceMark in print functions with an outputStream argument
-
- Closed
-
-
JDK-8244946 fatal error: memory leak: allocating without ResourceMark with -XX:+Verbose -Xlog:methodhandles
-
- Resolved
-
-
JDK-8223412 tier1 build failure after 8222893
-
- Closed
-
-
JDK-8223481 gtest/GTestWrapper.java failed due to "assert(ret == 0) failed: sem_post failed; error='Invalid argument' (errno=EINVAL)"
-
- Closed
-