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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Jul 13 19:56:12 UTC 2020 

Looks good to me!  There were so many variations of "perm gen" to look 
for.  Thanks for finding that one.
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