RFR 8194085: Obsolete the deprecated SafepointSynchronize flags and remove related code
harold seigel
harold.seigel at oracle.com
Fri Feb 23 15:41:41 UTC 2018
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.
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
>>>
More information about the hotspot-runtime-dev
mailing list