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

Mon Jul 13 19:43:13 UTC 2020

Greetings,

I've made minor tweaks based on Patricio's and Coleen's CR0 reviews.
Of course, while I was looking for safepoint related comments that
needed tweaking, I ran across a "perm gen" comment that needed to
be fixed and a misspelling of NoSafepointVerifier. I fixed both...

Fixing the "#ifdef ASSERT ... #endif" blocking touched a few files
so I've gone ahead and generated new webrevs.

Full webrev:

http://cr.openjdk.java.net/~dcubed/8246476-webrev/1_for_jdk16.full/

Incremental webrev:

http://cr.openjdk.java.net/~dcubed/8246476-webrev/1_for_jdk16.inc/

May I please have at least one re-review?

Thanks, in advance, for any comments, questions or suggestions.

Dan

On 7/13/20 3:23 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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
>>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>