RFR(S): 8221350 more monitor logging updates from Async Monitor Deflation project

[Daniel D. Daugherty]

On 3/24/19 10:02 PM, David Holmes wrote:
> Hi Dan,
>
> On 23/03/2019 6:23 am, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have a (S)mall patch extracted from the Async Monitor Deflation 
>> project
>> that is ready for code review.
>>
>> The short version of what this patch is about:
>>
>>      More monitor logging updates.
>>
>> The details are in the bug report:
>>
>>      JDK-8221350  monitor logging updates from Async Monitor 
>> Deflation project
>> https://bugs.openjdk.java.net/browse/JDK-8221350
>>
>> Here's the webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8221350-webrev/3-for-jdk13.more_monitor_logging/ 
>
>
> This mostly seems okay.

Thanks.


> One comment on the timer changes in synchronizer.cpp. Why not just 
> change this:
>
> 1711   timer.stop();
> 1712
> 1713   Thread::muxAcquire(&gListLock, "deflate_thread_local_monitors");
> 1714
> 1715   // Adjust counters
> 1716   counters->nInCirculation += thread->omInUseCount;
> 1717   thread->omInUseCount -= deflated_count;
> 1718   counters->nScavenged += deflated_count;
> 1719   counters->nInuse += thread->omInUseCount;
> 1720   counters->perThreadScavenged += deflated_count;
> 1721   // For now, we only care about cumulative per-thread deflation 
> time.
> 1722   counters->perThreadTimes += timer.seconds();
>
> to move the timer.stop() to after line 1720, rather than moving 
> outside the mux-block and reacquiring the mux again?

How about a slight tweak to that:

We'll move the timer.stop() and the perThreadTimes to just
before the muxRelease():

Diff relative to the baseline:

***************
*** 1708,1715 ****

     int deflated_count = 
deflate_monitor_list(thread->omInUseList_addr(), &freeHeadp, &freeTailp);

-   timer.stop();
-
     Thread::muxAcquire(&gListLock, "deflate_thread_local_monitors");

     // Adjust counters
--- 1717,1722 ----
***************
*** 1718,1725 ****
     counters->nScavenged += deflated_count;
     counters->nInuse += thread->omInUseCount;
     counters->perThreadScavenged += deflated_count;
-   // For now, we only care about cumulative per-thread deflation time.
-   counters->perThreadTimes += timer.seconds();

     // Move the scavenged monitors back to the global free list.
     if (freeHeadp != NULL) {
--- 1725,1730 ----
***************
*** 1730,1736 ****
--- 1735,1760 ----
       freeTailp->FreeNext = gFreeList;
       gFreeList = freeHeadp;
     }
+
+   timer.stop();
+   // Safepoint logging cares about cumulative perThreadTimes and
+   // we'll capture most of the cost, but not the muxRelease() which
+   // should be cheap.
+   counters->perThreadTimes += timer.seconds();
+
     Thread::muxRelease(&gListLock);


What do you think?

Dan


>
> Thanks,
> David
> -----
>
>>
>> This patch along with the current patch for Async Monitor Deflation
>> project have been through Mach5 tier[1-8] testing.
>>
>> I have been actively using this new logging code while debugging and
>> analyzing my port of the Async Monitor Deflation project code.
>>
>> Thanks, in advance, for any questions, comments or suggestions.
>>
>> Dan