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

[Daniel D. Daugherty]

On 1/28/19 4:32 PM, coleen.phillimore at oracle.com wrote:
>
> Hi Dan, This looks better, especially audit_and_print_stats.

Thanks! And thanks for the re-review!!


> My eyes are terrible, but it looks like this is 
> ObjectMonitor::verify_free()

This comment really confused me. I didn't remember an
ObjectMonitor::verify_free() so I was grep'ing all over the
place trying to find it.

After reading to the bottom of this email, I see that you
think this could be refactored into a new function called
ObjectMonitor::verify_free(). Now I feel less confused... :-)


> + 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;
> + } ...
>
> 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;
> + } ...

And another refactor recommendation... okay.


> And you do the same checks for the per-thread versions.  That's what I 
> noticed before.  I like the refactoring that you did but if you want 
> to do more, this is what I'd suggest.

In my first reply to David on this review thread, I wrote:

> Maybe after the above simplifications, other ways to make this
> function more digestible will become apparent... 

What you're suggesting fits right in with more simplifications
becoming apparent.


> ObjectMonitor doesn't have any verify functions and it seems like the 
> knowledge of these fields really belongs there.  Anyways, these files 
> are tightly coupled, so it's ok the way it is.  Maybe better not to 
> pull that thread right now.

It's kind of a toss up between ObjectMonitor.* and synchronizer.*.
Yes, ObjectMonitor.* defines all the fields and _some_ of the ops,
but there is a lot of logic in synchronizer.* that determines how
the fields are used.

I'll take a look at the possibility of more refactoring, but it
may not happen with this bug (8217659).

Dan


>
> thanks,
> Coleen
>
>
>
> On 1/26/19 10:10 PM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've updated the patch to resolve code review comments from David H.
>> and Coleen. I've also rebased the patch to the jdk/jdk repo at the
>> 'jdk-13+5' tag (plus the 8217658 patch); there were no merge surprises.
>>
>> Here's an incremental webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8217659-webrev/1-for-jdk13.inc/
>>
>> I've also generated a full webrev since the changes to synchronizer.cpp
>> are extensive and that's the easier way to review it:
>>
>> http://cr.openjdk.java.net/~dcubed/8217659-webrev/1-for-jdk13.full/
>>
>> This patch along with 8217658 have been through Mach5
>> builds-tier1,hs-tier1,jdk-tier1,hs-tier2,hs-tier3 testing with
>> no failures.
>>
>> Thanks, in advance, for any questions, comments or suggestions.
>>
>> Dan
>>
>> P.S.
>> I've attached sample output for running Hello.java with the
>> monitorinflation logging enabled at Info, Debug and Trace.
>>
>>
>> On 1/23/19 3:16 PM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I have a (S)mall/(M)edium patch extracted from the Async Monitor 
>>> Deflation
>>> project that is ready for code review. I'm calling this a 
>>> (S)mall/(M)edium
>>> because the logic changes are (S)mall, but the logging code is 
>>> tedious and
>>> there's a bunch of it in audit_and_print_stats() so (M)edium.
>>>
>>> The short version of what this patch is about:
>>>
>>>     This sub-task captures updates and additions to the
>>>     baseline monitor logging code.
>>>
>>> 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
>>>
>>> Here's the webrev:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8217659-webrev/0-for-jdk13/
>>>
>>> This patch along with the other two patches for Async Monitor Deflation
>>> project have been through Mach5 
>>> builds-tier1,hs-tier1,jdk-tier1,hs-tier2,hs-tier3
>>> testing and I'm currently running my stress testing kits on my 
>>> Linux-X64
>>> and Solaris-X64 servers.
>>>
>>> I have been actively using this new logging code while debugging my
>>> port of the Async Monitor Deflation project code.
>>>
>>> Thanks, in advance, for any questions, comments or suggestions.
>>>
>>> Dan
>>>
>>>
>>
>