RFR(S) 8242504: Enhance the system clock to nanosecond precision

[David Holmes]

Hi Dan,

Thanks for taking a look.

On 28/05/2020 1:09 am, Daniel D. Daugherty wrote:
> On 5/26/20 12:59 AM, David Holmes wrote:
>> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
>> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
> 
> src/hotspot/os/posix/os_posix.hpp
>      No comments.
> 
> src/hotspot/os/posix/os_posix.inline.hpp
>      No comments.
> 
> src/hotspot/os/posix/os_posix.cpp
>      old L1686: #ifdef NEEDS_LIBRT
>      old L1687       // Close librt if there is no monotonic clock.
>      old L1688       if (handle != RTLD_DEFAULT) {
>      old L1689         dlclose(handle);
>      old L1690       }
>      old L1691 #endif
>          I don't see any explanation in the bug or in the CR invite
>          about why this code is deleted when this preceding code
>          remains:
> 
>          L1658:   // For older linux we need librt, for other OS we can 
> find
>          L1659:   // this function in regular libc.
>          L1660: #ifdef NEEDS_LIBRT
>          L1661:   // We do dlopen's in this particular order due to bug 
> in linux
>          L1662:   // dynamic loader (see 6348968) leading to crash on exit.
>          L1663:   handle = dlopen("librt.so.1", RTLD_LAZY);
>          L1664:   if (handle == NULL) {
>          L1665:     handle = dlopen("librt.so", RTLD_LAZY);
>          L1666:   }
>          L1667: #endif

Sorry I should have mentioned this. In the existing code if we don't 
have CLOCK_MONOTONIC support we will never use the dynamic handles to 
clock_gettime and so we can close librt again (if we loaded it for 
clock_gettime).

With the new code we will always use clock_gettime so we can't unload 
librt (if we needed it for clock_gettime) just because there is no 
CLOCK_MONOTONIC.

The existing code (and thus the new code) is technically missing 
something here:

1675   if (clock_getres_func != NULL && clock_gettime_func != NULL) {
...
1693   }

There could be an else clause that closes librt if it was loaded. But 
the reality is that these functions are present in librt so we should 
never reach such an else clause. Closing the handle to librt is a minor 
"good citizen" act. This will all be moot in the not too distant future 
when we rely on these functions being in libc on all platforms.

> src/hotspot/os/linux/os_linux.cpp
>      old L1382:   return jlong(time.tv_sec) * 1000  + jlong(time.tv_usec 
> / 1000);
>      new L1390:     return jlong(time.tv_sec) * MILLIUNITS  +
>      new L1391:            jlong(time.tv_usec) / (MICROUNITS / MILLIUNITS);
> 
>      old L1390:   nanos = jlong(time.tv_usec) * 1000;
>      new L1407:     nanos = jlong(time.tv_usec) * (NANOUNITS / MICROUNITS);
>          Thanks for replacing the literal 1000s in the old code.
> 
> test/jdk/java/time/test/java/time/TestClock_System.java
>      L207:                                + " millisecond precision: 
> "+countBetterThanMillisPrecision+"/"+1000);
>      L209:                                + " microsecond precision: 
> "+countBetterThanMicrosPrecision+"/"+1000);
>         nit - need spaces around some of the '+' operators.

Fixed.

> test/micro/org/openjdk/bench/java/lang/SystemTime.java 
> test/micro/org/openjdk/bench/java/lang/Systems.java
>      No comments.
> 
> My only "concern" is the deletion of closing the librt handle.
> If you have a good explanation for that, then I'm good with this patch.

Thanks again. I'll wait for your thumbs up on the explanation.

David
-----

> Dan
> 
> 
> 
>>
>> This work was contributed by Mark Kralj-Taylor:
>>
>> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html 
>>
>>
>> On the hotspot side we change the Linux implementations of 
>> javaTimeMillis() and javaTimeSystemUTC() so that they use 
>> clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping 
>> with our conditional use of this API I added a new guard
>>
>> os::Posix::supports_clock_gettime()
>>
>> and refined the existing supports_monotonic_clock() so that we can 
>> still use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the 
>> future (hopefully very near future) we will simply assume these APIs 
>> always exist.
>>
>> On the core-libs side the existing test:
>>
>> test/jdk/java/time/test/java/time/TestClock_System.java
>>
>> is adjusted to track the precision in more detail.
>>
>> Finally Mark has added instantNowAsEpochNanos() to the benchmark 
>> previously known as
>>
>> test/micro/org/openjdk/bench/java/lang/Systems.java
>>
>> which I've renamed to SystemTime.java as Mark suggested. I agree 
>> having these three functions measured together makes sense.
>>
>> Testing:
>>   - test/jdk/java/time/test/java/time/TestClock_System.java
>>   - test/micro/org/openjdk/bench/java/lang/SystemTime.java
>>   - Tiers 1-3
>>
>> Thanks,
>> David
>