RFR 8194085: Obsolete the deprecated SafepointSynchronize flags and remove related code

David Holmes david.holmes at oracle.com
Fri Feb 23 02:16:03 UTC 2018


I should clarify expectation here. What I want to see is something like 
this:

310           //    to drive subsequent spin/SwitchThreadTo()/Sleep(N) 
decisions.
311
312           if (int(iterations) == -1) { // overflow - something is wrong
313             guarantee (PageArmed == 0, "invariant") ; // historical form
314           }

I recommend keeping the same guarantee condition so that it can be 
easily searched for in bug reports - even though it seems a little out 
of place given we don't arm the page here.

David

On 23/02/2018 11:53 AM, David Holmes wrote:
> On 23/02/2018 12:47 AM, Daniel D. Daugherty wrote:
>> On 2/21/18 11:49 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 22/02/2018 4:31 AM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this JDK-11 change to obsolete the 
>>>> SafepointSpinBeforeYield, DeferThrSuspendLoopCount, and 
>>>> DeferPollingPageLoopCount options and related code.  With this 
>>>> change, these options are still accepted on the command line but 
>>>> have no affect other than to generate these warning messages:
>>>>
>>>>     Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
>>>>     SafepointSpinBeforeYield; support was removed in 11.0
>>>>     Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
>>>>     DeferThrSuspendLoopCount; support was removed in 11.0
>>>>     Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
>>>>     DeferPollingPageLoopCount; support was removed in 11.0
>>>
>>> Just for the record these were once useful tuning values for 
>>> investigating safepoint performance issues in the field.
>>>
>>>> Open Webrev: 
>>>> http://cr.openjdk.java.net/~hseigel/bug_8194085/webrev/index.html
>>>
>>> This one is a bit messier than I had envisaged: -Xconcurrentio ?? 
>>> Wow! :) Definitely overlooked that one. It should be deprecated in 11 
>>> for certain.
>>>
>>> I have one problem with a change in safepoint.cpp, the removal of this:
>>>
>>>  312           if (SafepointMechanism::uses_global_page_poll() && 
>>> int(iterations) == DeferPollingPageLoopCount) {
>>>  313             guarantee (PageArmed == 0, "invariant") ;
>>>  314             PageArmed = 1 ;
>>>  315             os::make_polling_page_unreadable();
>>>  316           }
>>>
>>> This block triggers a guarantee failure if we fail to reach a 
>>> safepoint and the number of iterations overflows to reach -1 (the 
>>> value of DeferPollingPageLoopCount). That is the only thing that 
>>> stops the VM from just hanging when we have an issue reaching 
>>> safepoints (like all of the "counted loop" problems!) and is very 
>>> valuable in clearly identifying the problem. So I strongly recommend 
>>> that this is restored with an explicit check against -1.
>>
>> Hmmmm... I guess I'm not seeing why the changes here don't
>> solve that problem:
>>
>>>   190     if (SafepointMechanism::uses_global_page_poll()) {
>>>   191       // Make interpreter safepoint aware
>>>   192       Interpreter::notice_safepoints();
>>>   193
>>> 194 if (DeferPollingPageLoopCount < 0) {
>>>   195         // Make polling safepoint aware
>>>   196         guarantee (PageArmed == 0, "invariant") ;
>>>   197         PageArmed = 1 ;
>>>   198         os::make_polling_page_unreadable();
>>>   199       }
>>> 200 }
>>
>> So L194 and L200 are deleted and the same block is always
>> run when SafepointMechanism::uses_global_page_poll() is true
>> and this happens before the "L221: while(still_running > 0) {"
>>
>> Did I miss something?
> 
> That block is executed once.
> 
> The deleted block is executed in the loop while trying to reach the 
> safepoint and so detects excessive looping because we can't get to the 
> safepoint.
> 
> David
> 
>> Dan
>>
>>
>>>
>>> ---
>>>
>>> src/hotspot/share/runtime/safepoint.hpp
>>>
>>> +   static void set_throughput_mode() {
>>> +     _defer_thr_suspend_loop_count = 1;
>>> +   }
>>>
>>> First can you add a comment before this that it is only used for 
>>> -Xconcurrentio support.
>>>
>>> Second, the name seems odd - throughput mode? How about 
>>> set_concurrentio_mode() ? Or even just 
>>> set_defer_thr_suspend_loop_count() ?
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8194085
>>>>
>>>> The change was tested with Mach5 tiers 1 and 2 on all Mach5 
>>>> platforms and tiers 3-5 in Linux-X64.
>>>>
>>>> Thanks, Harold
>>


More information about the hotspot-runtime-dev mailing list