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

[David Holmes]

Thanks Dan!

David

On 28/05/2020 12:52 pm, Daniel D. Daugherty wrote:
>> I'll wait for your thumbs up on the explanation. 
> 
> I'm good with the explanation. Thanks!
> 
> Dan
> 
> 
> On 5/27/20 10:08 PM, David Holmes wrote:
>> 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
>>>
>