RFR: 8220795: Introduce TimedYield utility to improve time-to-safepoint on Windows

[Robin Westberg]

Hi David,

> On 23 Apr 2019, at 03:39, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Robin,
> 
> Sorry, now Easter break got in the way :)
> 
> <trimming>
> 
> On 17/04/2019 11:55 pm, Robin Westberg wrote:
>>> On 12 Apr 2019, at 11:15, David Holmes <david.holmes at oracle.com> wrote:
>>> I'd prefer to fix a windows problem, just on windows. I'm not hung up on having sleep in the name, but if you prefer timed_yield to naked_short_nanosleep then that's fine (and avoids people wondering what the "naked" part means).
>>> 
>>> If we need the TimedYield capability in the future then lets revisit that then.
>> Sure, here’s a lighter version of this change that changes the Windows implementation of naked_short_nanosleep, with a few adjustments to some assumptions in the waiting-for-safepoint backoff strategy.
>> Still passes tier1, with the same performance improvements on Windows (and no obvious regressions on Linux).
>> New webrev:
>> https://cr.openjdk.java.net/~rwestberg/8220795/webrev.02/
> 
> Windows changes look fine - thanks.
> 
> Safepoint backoff change seems okay but what affect does it have on performance on non-Windows? (javaTimeNanos can sometimes be expensive)

In the case where we have to wait for threads to stop it shouldn't matter, it will be insignificant compared to the time spent actually sleeping. But in the case where all threads manage to stop before we resort to waiting there is indeed an unnecessary call to javaTimeNanos - I have not observed any measurable difference, but I’ve reworked the code a little bit to avoid this just in case:

Full webrev: https://cr.openjdk.java.net/~rwestberg/8220795/webrev.03/
Incremental: https://cr.openjdk.java.net/~rwestberg/8220795/webrev.02-03/

Best regards,
Robin

> 
> Thanks,
> David
> 
>> Best regards,
>> Robin
>>> 
>>> Thanks,
>>> David
>>> -----
>>> 
>>>> Best regards,
>>>> Robin
>>>>> 
>>>>> ?
>>>>> 
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>> 
>>>>> 
>>>>>> Best regards,
>>>>>> Robin
>>>>>>> On 5 Apr 2019, at 13:54, Robin Westberg <robin.westberg at oracle.com> wrote:
>>>>>>> 
>>>>>>> Hi David,
>>>>>>> 
>>>>>>>> On 5 Apr 2019, at 12:10, David Holmes <david.holmes at oracle.com> wrote:
>>>>>>>> 
>>>>>>>> On 5/04/2019 7:53 pm, Robin Westberg wrote:
>>>>>>>>> Hi David,
>>>>>>>>> Thanks for taking a look!
>>>>>>>>>> On 5 Apr 2019, at 10:49, David Holmes <david.holmes at oracle.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> Hi Robin,
>>>>>>>>>> 
>>>>>>>>>> On 5/04/2019 6:05 pm, Robin Westberg wrote:
>>>>>>>>>>> Hi all,
>>>>>>>>>>> Please review the following change that modifies the way safepointing waits for other threads to stop. As part of JDK-8203469, os::naked_short_nanosleep was used for all platforms. However, on Windows, this function has millisecond granularity which is too coarse and caused performance regressions. Instead, we can take advantage of the fact that Windows can tell us if anyone else is waiting to run on the current cpu. For other platforms the original waiting is used.
>>>>>>>>>> 
>>>>>>>>>> Can't you just make the new code the implementation of os::naked_short_nanosleep on Windows and avoid adding yet another sleep/yield abstraction? If Windows nanosleep only has millisecond granularity then it's broken.
>>>>>>>>> Right, I considered that approach, but it's not obvious to me what semantics would be appropriate for that. Depending on whether you want an accurate sleep or want other threads to make progress, it could conceivably be either implemented as pure spinning, or spinning combined with yielding (either cpu-local or Sleep(0)-style).
>>>>>>>>> That said, perhaps os_naked_short_nanosleep should be removed, the only remaining usage is in spinYield, which turns to sleeping after giving up on yielding. For windows, it may be more appropriate to switch to Sleep(0) in that case.
>>>>>>>> 
>>>>>>>> I definitely don't think we need to introduce another abstraction (though I'm okay with replacing one with a new one).
>>>>>>> 
>>>>>>> Okay, then I’d prefer to drop the os::naked_short_nanosleep. As I see it, you would use SpinYield when waiting for something like a CAS race, and TimedYield when waiting for a thread rendezvous.
>>>>>>> 
>>>>>>> I did try to make TimedYield into a different “policy” for the existing SpinYield class at first, but didn’t quite feel like I found a nice API for it. So perhaps it's fine to just have them as separate classes.
>>>>>>> 
>>>>>>>> It was probably discussed at the time but the naked_short_nanosleep on Windows definitely seems to have far too much overhead - even having to create a new waitable-timer each time. Do we know if the overhead is actually the resolution of the timer or whether it's all the extra work that method needs to do? I should try to dig up the email on that. :)
>>>>>>> 
>>>>>>> Yes, as far as I know the best possible sleep resolution you can obtain on Windows is one millisecond (perhaps 0.5ms in theory if you go directly for NtSetTimerResolution). The SetWaitableTimer approach is the best though to actually obtain 1ms resolution, plain Sleep(1) usually sleeps around 2ms.
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> Robin
>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> 
>>>>>>>>> Best regards,
>>>>>>>>> Robin
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>> 
>>>>>>>>>>> Various benchmarks (specjbb2015, specjm2008) show better numbers for Windows and no regressions for Linux.
>>>>>>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8220795
>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rwestberg/8220795/webrev.00/
>>>>>>>>>>> Testing: tier1
>>>>>>>>>>> Best regards,
>>>>>>>>>>> Robin