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

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Jul 13 19:57:26 UTC 2020 

On 7/13/20 3:56 PM, coleen.phillimore at oracle.com wrote:
>
> Looks good to me!

Thanks for the fast re-review!


> There were so many variations of "perm gen" to look for.  Thanks for 
> finding that one.

No problem. Thanks for letting me fix it as part of this work.

Dan


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

More information about the hotspot-runtime-dev mailing list