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

[Daniel D. Daugherty]

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/

src/hotspot/os/posix/os_posix.cpp
     No comments.

src/hotspot/os/windows/os_windows.cpp
     L5253:     delete phri; //if it is NULL, harmless
         nit - need a space between '//' and 'if'.
         (Not really your nit since you just moved the code, but...)

src/hotspot/share/runtime/os.cpp
     Thanks for adding more comments to the os::sleep() function.

     nit - extra blank lines: 1887, 1923, 1924

     L1904:       if (millis <= 0) {
     L1905:         return OS_OK;
     L1906:       }
         If you change this if-statement to:

             if (millis <= 0) break;

         then you'll match the logic on:

             L1945:       if (millis <= 0) break ;

         and the new return you added here will also be used:

             L1926     return OS_OK;

Thumbs up! My comments are only nits so feel free to make
those changes or not. If you do make those changes, I don't
need to see another webrev.

The bug report was (again) an interesting read... as usual with time
on Windows, you have had quite the adventure. Good job on the notes.

Dan


>
> 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
> -----