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

[Daniel D. Daugherty]

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