On Tue, 18 Apr 2023 19:38:12 GMT, Aleksey Shipilev <sh...@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
