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

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~dcubed/8217659-webrev/0-for-jdk13/src/hotspot/share/runtime/synchronizer.cpp.frames.html

1426 if (log_is_enabled(Trace, monitorinflation)) {
1427         if (object->is_instance()) {
1428           ResourceMark rm(Self);
1429 log_trace(monitorinflation)("inflate(has_locker): "
1430                                       "object=" INTPTR_FORMAT ", mark=" INTPTR_FORMAT ", type='%s'",
1431                                       p2i(object), p2i(object->mark()),
1432                                       object->klass()->external_name());
1433         }
1434       }

It seems like extra overhead to ask if logging is enabled in both 1426 
and internally in 1429.  It seems that if object->is_instance() is a 
cheap test, 1426 is unnecessary.  I realize it was like this.   Same at 
1477, 1532.  I don't think ResourceMarks are expensive either.


1653 if (log_is_enabled(Info, monitorinflation)) {
1654 log_info(monitorinflation)("deflating global idle monitors, %3.7f 
secs, %d monitors", timer.seconds(), deflated_count);
1655 }
1656 } else {
1657 if (log_is_enabled(Debug, monitorinflation)) {
1658 log_debug(monitorinflation)("deflating global idle monitors, %3.7f 
secs, 0 monitors", timer.seconds());
1659 }


log_info/log_debug also ask if log_is_enabled, so the log_is_enabled 
test isn't necessary here, and 1673, 1677 and 1664.  I think a lot of 
these were early logging changes that weren't caught then.

Duplicating David's comment

audit_and_print_stats

can create a LogStream log and use that instead of doing log_info/etc.

Instead of

2048 if (IncludeInUseMonitorDetailsInLogMsgs) {

You can add a tag to the logging, so that you can say 
-Xlog:monitorinflation+inuse or some such tag.

It's not worth adding another command line option when logging gives you 
the ability to be specific about what output you want.

Lastly, I didn't look at what you're verifying and logging but maybe the 
~300 line function could be split and you could add more tags in the 
future to taylor what specific information and verification you are 
looking for.   It looks there's a bit of duplication in the logging 
messages.

Coleen

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
>