RFR: 6313903: Thread.sleep(3) might wake up immediately on windows

[David Holmes]

Hi Roger,

Thanks for taking a look.

On 4/09/2019 12:30 am, Roger Riggs wrote:
> Hi David,
> 
> In os.cpp:1893, can the double read of javaTimeNanos be avoided on the 
> first iteration?

Note this is the existing code and I'm deliberately trying not to change 
anything in that regard.

> Set newtime = prevtime before the loop and re-read it after the call to 
> park(:1918)

I can't quite see the full change you propose here. The code has to read 
the new time, check for timeout, update prevtime - in that order, before 
doing the actual wait. If all that is moved after the wait then we 
aren't giving interrupt precedence. If we move the interrupt check after 
the wait as well then we need to add an additional interrupt check 
before the loop for the first time through. (I actually had a version 
that did this and decided it was too disruptive as part of this code 
relocation.)

> Negible nanotime should have elapsed between 1881 and 1893.
> Ditto the non-interruptable code in 1936-1947.

Note the non-interuptible code will disappear in my next RFE.

> (Unless javaTimeNanos is free?).

Afraid not.

Thanks,
David
-----

> $.02, Roger
> 
> 
> 
> 
> On 9/2/19 2:06 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-6313903
>> webrev: http://cr.openjdk.java.net/~dholmes/6313903/webrev/
>>
>> This is a fix to a very long term problem on Windows that really 
>> should have been fixed many years ago: the Windows timed-wait routines 
>> can return early by up to a clock tick** (which itself could be as 
>> long as 16ms). The fix for Thread.sleep is to track the elapsed time 
>> (via nanoTime) and simply adjust and repeat the sleep if we were going 
>> to return earlier than requested. This is the same as what we have 
>> been doing on non-Windows for many years.
>>
>> ** whether or not you observe this seems highly dependent on hardware 
>> - see bug report for observed behaviours.
>>
>> The additional motivation for fixing this now is to reduce the amount 
>> of platform specific sleep/interrupt code and to allow changes to the 
>> shared interrupt code. There are some additional RFE's in the pipeline 
>> for that.
>>
>> This fix does the following:
>> - moves the os_posix.cpp os::sleep implementation to os.cpp to be 
>> shared by all platforms and deletes the os_windows.cpp os::sleep 
>> implementation
>>   - there are some additional comments added and the JavaThread 
>> assertion is moved to the head of the "interruptible" code path
>> - updates os_windows.cpp in these additional ways:
>>   - updates the commentary about creating the interrupt_event (which 
>> is now only used by JDK library code for use by Process.waitFor)
>>   - updates os::interrupt to also unpark the _SleepEvent
>>   - moves the use of the HighResolutionInterval utility class from the 
>> os::sleep code to the PlatformEvent::park code, so that sleep on 
>> Windows will still make adjustments to the clock tick if needed (it 
>> also means that Object.wait() calls will now make the same 
>> adjustments**).
>>
>> ** This is a change behaviour but I'm not concerned about there being 
>> any adverse effects from this. On many systems the code will have no 
>> affect as the system will be using the 1ms clock tick all the time 
>> anyway (e.g. my laptops do this). On systems with e.g. 15ms tick, what 
>> you will now see is Object.wait returning on time rather than much 
>> later than requested e.g. before this fix a timed-wait of 1ms returned 
>> in 15.14ms, but after the fix it was 2.3ms. (Note that this 
>> observation seems contrary to the general statement that windows 
>> timed-wait event operations can return early by up to a clock tick - 
>> as I said it seems highly hardware dependent.)
>>
>> Testing: tiers 1-3
>>          custom testing to look at sleep and wait times, as per bug 
>> report.
>>
>> Thanks,
>> David
>> -----
>