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

[David Holmes]

Hi Harold,

On 24/02/2018 1:41 AM, harold seigel wrote:
> Thanks Coleen, Dan, and David for your comments!
> 
> Please review this updated webrev for bug JDK-8194085.
> 
>     http://cr.openjdk.java.net/~hseigel/bug_8194085.2/webrev/index.html
> 
> This updated webrev contains the changes suggested by Coleen, Dan, and 
> David.

src/hotspot/share/runtime/arguments.cpp

It seems that when we actually obsolete a flag we should move its entry 
down into the "obsolete flags" section of the table. No need to see 
updated webrev for that.

Other updates are fine to me.

Thanks,
David
-----

> Thanks, Harold
> 
> On 2/22/2018 9:16 PM, David Holmes wrote:
>> 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
>>>>
>