RFR(S/M): 8217659 monitor_logging updates from Async Monitor Deflation project

[Daniel D. Daugherty]

I've snipped this reply down to just the part about refactoring...


On 1/28/19 5:22 PM, Daniel D. Daugherty wrote:
> On 1/28/19 5:15 PM, coleen.phillimore at oracle.com wrote:
>>>
>>>> My eyes are terrible, but it looks like this is 
>>>> ObjectMonitor::verify_free()
>>>
>>>> + if (n->is_busy()) {
>>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": free global 
>>>> monitor "
>>>> + "must not be busy.", p2i(n));
>>>> + *error_cnt_p = *error_cnt_p + 1;
>>>> + }
>>>> + if (n->header() != NULL) {
>>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": free global 
>>>> monitor "
>>>> + "must have NULL _header field: _header=" INTPTR_FORMAT,
>>>> + p2i(n), p2i(n->header()));
>>>> + *error_cnt_p = *error_cnt_p + 1;
>>>> + }
>>>> + if (n->object() != NULL) {
>>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": free global 
>>>> monitor "
>>>> + "must have NULL _object field: _object=" INTPTR_FORMAT,
>>>> + p2i(n), p2i(n->object()));
>>>> + *error_cnt_p = *error_cnt_p + 1;
>>>> + } ...

I have refactored the common code for "verify free" into:

+// Check a free monitor entry; log any errors.
+void ObjectSynchronizer::ck_free_entry(JavaThread * jt, ObjectMonitor * n,
+                                       outputStream * out, int 
*error_cnt_p) {

And here's what one of the checks looks like. I tried to put it all into
a single out->print_cr() call with conditional inclusion of format strings
and parameters, but the compiler really hated it. So I went with:

+  if (n->is_busy()) {
+    if (jt != NULL) {
+      out->print_cr("ERROR: jt=" INTPTR_FORMAT ", monitor=" INTPTR_FORMAT
+                    ": free per-thread monitor must not be busy.", p2i(jt),
+                    p2i(n));
+    } else {
+      out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": free global 
monitor "
+                    "must not be busy.", p2i(n));
+    }
+    *error_cnt_p = *error_cnt_p + 1;
+  }



>>>> And this is ObjectMonitor::verify_in_use()
>>>>
>>>> + if (n->header() == NULL) {
>>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": in-use global 
>>>> monitor "
>>>> + "must have non-NULL _header field.", p2i(n));
>>>> + *error_cnt_p = *error_cnt_p + 1;
>>>> + }
>>>> + if (n->object() == NULL) {
>>>> + out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": in-use global 
>>>> monitor "
>>>> + "must have non-NULL _object field.", p2i(n));
>>>> + *error_cnt_p = *error_cnt_p + 1;
>>>> + } ...

I have refactored the common code for "verify in use" into:

+// Check an in-use monitor entry; log any errors.
+void ObjectSynchronizer::ck_in_use_entry(JavaThread * jt, ObjectMonitor 
* n,
+                                         outputStream * out, int 
*error_cnt_p) {

And here's what one of the checks looks like:

+  if (n->header() == NULL) {
+    if (jt != NULL) {
+      out->print_cr("ERROR: jt=" INTPTR_FORMAT ", monitor=" INTPTR_FORMAT
+                    ": in-use per-thread monitor must have non-NULL 
_header "
+                    "field.", p2i(jt), p2i(n));
+    } else {
+      out->print_cr("ERROR: monitor=" INTPTR_FORMAT ": in-use global 
monitor "
+                    "must have non-NULL _header field.", p2i(n));
+    }
+    *error_cnt_p = *error_cnt_p + 1;
+  }

Hopefully this is the last refactor for this bug fix... :-)


Here's an incremental webrev:

http://cr.openjdk.java.net/~dcubed/8217659-webrev/2-for-jdk13.inc/

Here's a full webrev:

http://cr.openjdk.java.net/~dcubed/8217659-webrev/2-for-jdk13.full/

Another Mach5 builds-tier1,hs-tier1,jdk-tier1,hs-tier2,hs-tier3 test
run is in process...

Thanks, in advance, for any questions, comments or suggestions.

Dan

> The details are in the bug report:
>
>     JDK-8217659 monitor_logging updates from Async Monitor Deflation 
> project
> https://bugs.openjdk.java.net/browse/JDK-8217659