RFR(S/M): 8246476: remove AsyncDeflateIdleMonitors option and the safepoint based deflation mechanism

[Daniel D. Daugherty]

Hi Coleen,

Thanks for jumping in on this code review!


On 7/13/20 1:21 PM, coleen.phillimore at oracle.com wrote:
>
> http://cr.openjdk.java.net/~dcubed/8246476-webrev/0_for_jdk16/src/hotspot/share/runtime/objectMonitor.inline.hpp.udiff.html 
>
> http://cr.openjdk.java.net/~dcubed/8246476-webrev/0_for_jdk16/src/hotspot/share/runtime/synchronizer.cpp.udiff.html 
>
>
> +#ifdef ASSERT
>    void* prev = Atomic::load(&_owner);
> - ADIM_guarantee(prev == old_value, "unexpected prev owner=" 
> INTPTR_FORMAT
> +#endif
> + assert(prev == old_value, "unexpected prev owner=" INTPTR_FORMAT
>                   ", expected=" INTPTR_FORMAT, p2i(prev), 
> p2i(old_value));
>
> Just a nit but these patterns look really strange.  Can you put the 
> #endif on the other side of the assert?

I can do that. In previous reviews of other fixes, I think I've had 
assert()
calls inside #ifdef ASSERT ... #endif blocks and have been asked to move
them out since it's redundant.

I'll plan to make that change, but I'll keep an eye open for complaints
from other reviewers before I push...


> http://cr.openjdk.java.net/~dcubed/8246476-webrev/0_for_jdk16/src/hotspot/share/runtime/safepoint.cpp.udiff.html 
>
>
> if 
> (_subtasks.try_claim_task(SafepointSynchronize::SAFEPOINT_CLEANUP_DEFLATE_MONITORS)) 
> { ... + ObjectSynchronizer::do_safepoint_work();
>
>
> Why do we still have this to trigger the ServiceThread when we're 
> going to only wait GuaranteedSafepointInterval before checking for 
> monitor deflation anyway?

When a cleanup safepoint triggers an async deflation request, it
counts as a direct async deflation request so it is not subject
to the AsyncDeflationInterval limit. In other words, when a cleanup
safepoint asks, we honor the request regardless of when the most
recent async deflation request was finished.

When we do a periodic wake-up and check to see if an async deflation
is needed, we'll only honor that request if it has been longer than
AsyncDeflationInterval since we last completed an async deflation
cycle. So the periodic invocation mechanism has a limit so that we
don't swamp the ServiceThread.


> Why have this in a safepoint cleanup task?

I figured I would leave it because there are plans to get rid of the
safepoint cleanup mechanism and whoever does that work can deal with
any comments in the code that talk about cleanup safepoints and the
like. That person will also have to evaluate and deal with whether
the safepoint cleanup task removal causes unacceptable issues in the
system.


> The code deletion is really nice.

Thanks!

Dan


>
> Thanks,
> Coleen
>
>
> On 7/13/20 12:08 PM, Daniel D. Daugherty wrote:
>> On 7/13/20 12:05 PM, Patricio Chilano wrote:
>>> Hi Dan,
>>>
>>> Changes look good to me!
>>
>> Thanks!
>>
>>
>>> In synchronizer.cpp we have this comment about ObjectMonitor lifecycle:
>>>
>>> // Inflation unlinks monitors from om_list_globals._free_list or a 
>>> per-thread
>>> // free list and associates them with objects. Deflation -- which 
>>> occurs at
>>> // STW-time or asynchronously -- disassociates idle monitors from 
>>> objects.
>>> // Such scavenged monitors are returned to the 
>>> om_list_globals._free_list.
>>>
>>> With all the older code removed, are there still cases where we do 
>>> deflations at safepoint?
>>
>> Good catch! I need to adjust that comment. I'll look for others also.
>>
>> Dan
>>
>>
>>>
>>> Thanks!
>>> Patricio
>>> On 7/13/20 10:47 AM, Daniel D. Daugherty wrote:
>>>> Hi David,
>>>>
>>>> Thanks for the review!
>>>>
>>>> I need a second review folks... any takers?
>>>>
>>>> Dan
>>>>
>>>>
>>>> On 7/12/20 10:57 PM, David Holmes wrote:
>>>>> Hi Dan,
>>>>>
>>>>> This all looks good to me.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>> On 8/07/2020 5:51 pm, David Holmes wrote:
>>>>>> On 8/07/2020 7:21 am, Daniel D. Daugherty wrote:
>>>>>>> Ping! Any takers??? Code deletion should be really appealing here!!
>>>>>>
>>>>>> Sorry Dan didn't get to it before vacation. But if you can wait 
>>>>>> till Monday ...
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>> On 7/6/20 12:35 PM, Daniel D. Daugherty wrote:
>>>>>>>> Greetings,
>>>>>>>>
>>>>>>>> It's time to remove the AsyncDeflateIdleMonitors option from 
>>>>>>>> JDK16. We can
>>>>>>>> also get rid of the safepoint based deflation mechanism since 
>>>>>>>> turning off
>>>>>>>> async deflation (-XX:-AsyncDeflateIdleMonitors) was the only 
>>>>>>>> way left to
>>>>>>>> use it.
>>>>>>>>
>>>>>>>> This is marked as an "S/M" review because the number of 
>>>>>>>> touched/deleted
>>>>>>>> lines makes it a Medium review, but the number of 
>>>>>>>> touched/changed lines
>>>>>>>> (outside of the deletions) makes it a Small review. It's 
>>>>>>>> actually a pretty
>>>>>>>> fast read... :-)
>>>>>>>>
>>>>>>>> Here's the bug ID:
>>>>>>>>
>>>>>>>>     JDK-8246476 remove AsyncDeflateIdleMonitors option and the 
>>>>>>>> safepoint
>>>>>>>>                 based deflation mechanism
>>>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8246476
>>>>>>>>
>>>>>>>> Here's the webrev URL:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~dcubed/8246476-webrev/0_for_jdk16/
>>>>>>>>
>>>>>>>> The webrev is baselined on Thomas S's fix for 8248650 which is 
>>>>>>>> jdk-16+4
>>>>>>>> plus a dozen or so changesets.
>>>>>>>>
>>>>>>>> This change has been tested with Mach5 Tier[1-3],4,5,6,7,8 and 
>>>>>>>> there are
>>>>>>>> no regressions (and very few known failures). My inflation 
>>>>>>>> stress testing
>>>>>>>> is still in process. I had to restart that testing after a 
>>>>>>>> thunderstorm
>>>>>>>> related power failure took down my servers in Florida. Sigh...
>>>>>>>>
>>>>>>>> Thanks, in advance, for any comments, questions, or suggestions.
>>>>>>>>
>>>>>>>> Dan
>>>>>>>
>>>>
>>>
>>
>