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

[coleen.phillimore at oracle.com]

Hi Dan, This looks better, especially audit_and_print_stats.

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

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 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.   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.

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
>>
>>
>