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

[Daniel D. Daugherty]

Thanks! Don't think this one qualifies as trivial so I'll
wait for another reviewer...

Dan


On 3/25/19 6:30 PM, David Holmes wrote:
> Hi Dan,
>
> Your tweak looks good.
>
> Thanks,
> David
>
> On 26/03/2019 7:25 am, Daniel D. Daugherty wrote:
>> 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
>>