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

[David Holmes]

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
>