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
