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

Mon Jul 13 19:23:47 UTC 2020

On 7/13/20 2:49 PM, Daniel D. Daugherty wrote:
> 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.

Ok, I see it now, thanks!  If we had a lot of safepoints, we would swamp 
the service thread also, but we don't have a lot of safepoints anymore.

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

I think we're still going to need safepoint cleanup for the other 
things, but ok, someone can evaluate this later.  There's also this RFE, 
which I linked to this just now.

https://bugs.openjdk.java.net/browse/JDK-8227060

Thanks,
Coleen
>
>> 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
>>>>>>>>
>>>>>
>>>>
>>>
>>
>