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

[David Holmes]

Hi Dan,

Thanks for the review.

On 4/09/2019 6:10 am, Daniel D. Daugherty wrote:
> 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...)

I'll fix.

> src/hotspot/share/runtime/os.cpp
>      Thanks for adding more comments to the os::sleep() function.
> 
>      nit - extra blank lines: 1887, 1923, 1924

Will fix.

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

Those stylistic differences bothered me as well but I didn't want to 
make too many changes to the existing code, and the non-interruptible 
part will be going away soon, so I'll leave future cleanup till then.

>          and the new return you added here will also be used:
> 
>              L1926     return OS_OK;

That may have been a leftover from an earlier, much more disruptive 
version - will check.

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

Ok.

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

Thanks again.

David
-----

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