RFR: 8305092: Improve Thread.sleep(millis, nanos) for sub-millisecond granularity [v3]

[David Holmes]

On Tue, 18 Apr 2023 19:38:12 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Java API has the `Thread.sleep(millis, nanos)` method exposed to users. The documentation for that method clearly says the precision and accuracy are dependent on the underlying system behavior. However, it always rounds up `nanos` to 1ms when doing the actual sleep. This means users cannot do the micro-second precision sleeps, even when the underlying platform allows it. Sub-millisecond sleeps are useful to build interesting primitives, like the rate limiters that run with >1000 RPS.
>> 
>> When faced with this, some users reach for more awkward APIs like `java.util.concurrent.locks.LockSupport.parkNanos`. The use of that API for sleeps is not in line with its intent, and while it "seems to work", it might have interesting interactions with other uses of `LockSupport`. Additionally, these "sleeps" are no longer visible to monitoring tools as "normal sleeps", e.g. as `Thread.sleep` events. Therefore, it would be prudent to improve current `Thread.sleep(millis, nanos)` for sub-millisecond granularity. 
>> 
>> Fortunately, the underlying code is almost ready for this, at least on POSIX side. I skipped Windows paths, because its timers are still no good. Note that on both Linux and MacOS timers oversleep by about 50us. I have a few ideas how to improve the accuracy for them, which would be a topic for a separate PR.
>> 
>> Additional testing:
>>   - [x] New regression test
>>   - [x] New benchmark
>>   - [x] Linux x86_64 `tier1`
>>   - [x] Linux AArch64 `tier1`
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix gtests

You seemed to have missed:

./cpu/x86/rdtsc_x86.cpp:    JavaThread::current()->sleep(FT_SLEEP_MILLISECS);
./share/compiler/compileBroker.cpp:  sleep(DeoptimizeObjectsALotInterval);

so not sure how this is building ???

A few other comments below.

Thanks

src/hotspot/os/posix/os_posix.cpp line 1545:

> 1543: 
> 1544: int PlatformEvent::park_nanos(jlong nanos) {
> 1545:   assert(0 <= nanos, "nanos are in range");

`nanos` should never be zero else you call the untimed park.

src/hotspot/os/posix/park_posix.hpp line 57:

> 55:   void park();
> 56:   int  park_millis(jlong millis);
> 57:   int  park_nanos(jlong nanos);

Still not sure we need this API split but if we keep `park(jlong millis)` and just add `park_nanos(jlong nanos)` then you can avoid touching so many places in the code.

src/hotspot/os/windows/os_windows.cpp line 5253:

> 5251: 
> 5252: int PlatformEvent::park_nanos(jlong nanos) {
> 5253:   assert(0 <= nanos, "nanos are in range");

`nanos` should never be zero else you call the untimed park.

src/hotspot/os/windows/os_windows.cpp line 5257:

> 5255:   // Windows timers are still quite unpredictable to handle sub-millisecond granularity.
> 5256:   // Instead of implementing this method, fall back to the millisecond sleep, treating
> 5257:   // any positive requested nanos as a full millisecond.

Is this how the code currently works?

src/hotspot/os/windows/os_windows.cpp line 5259:

> 5257:   // any positive requested nanos as a full millisecond.
> 5258:   jlong millis = align_up(nanos, NANOSECS_PER_MILLISEC) / NANOSECS_PER_MILLISEC;
> 5259:   assert(nanos == 0 || millis != 0, "Only pass zero millis on zero nanos");

Not sure what this is trying to check.

Nit: s/on/or/

src/hotspot/share/runtime/javaThread.hpp line 1145:

> 1143: public:
> 1144:   bool sleep_millis(jlong millis);
> 1145:   bool sleep_nanos(jlong nanos);

I prefer just one sleep that takes nanos.

src/java.base/share/classes/java/lang/Thread.java line 576:

> 574:         long millis = NANOSECONDS.toMillis(nanos);
> 575:         nanos -= MILLISECONDS.toNanos(millis);
> 576:         sleep(millis, (int)nanos);

This double conversion seems  a bit kludgy - why not just keep the vthread check and call `sleep0(nanos)`?

test/jdk/java/lang/Thread/SleepSanity.java line 75:

> 73:     }
> 74: 
> 75:     private static void testTimes(TestCase t, long min, long max) throws Exception {

Not obvious without reading all the code that min and max are in ms. `msMin` and `msMax` might be clearer.

test/jdk/java/lang/Thread/SleepSanity.java line 84:

> 82:     }
> 83: 
> 84:     private static void testTimeout(TestCase t, long timeout) throws Exception {

Suggestion: s/timeout/millis/ again so unit is clear.

-------------

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13225#pullrequestreview-1391285246
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170851742
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170827783
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170840798
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170824754
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170841030
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170846464
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170831695
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170835700
PR Review Comment: https://git.openjdk.org/jdk/pull/13225#discussion_r1170835903