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

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

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

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

[Daniel D. Daugherty]

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

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.

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.

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

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

[David Holmes]

Hi Mark,

On 27/05/2020 2:15 am, Mark Kralj-Taylor wrote:

David,
Thanks for taking this enhancement, and making it work on the older 
glibc (pre 2.17) Linux platforms currently supported by openjdk.


I like that it is a small change to split the JVM startup check on 
availability of Posix clock_gettime/getres() APIs and then if 
additionally CLOCK_MONOTONIC is supported.


Glad to hear it. I had originally also envisaged something more complex 
so the simplicity here is appealing. Hopefully for 16 (or else 17) we 
can get rid of all the legacy support.


On Daniel's comment on the spec. Yes the nice part of this is that the 
JavaDoc on Clock.systemUTC() is worded to allow higher clock resolution 
than system.currentTimeMillis().


Please let me know if I can help any more on this.


I think we are set. Your very detailed write-up and the changes to the 
test and benchmark were a great assist. You've set a very high bar on 
your first OpenJDK contribution. :)


Now I just need someone from hotspot-runtime to sign-off on the VM side.

Cheers,
David


Mark

On Tue, 26 May 2020 at 16:35, Daniel Fuchs > wrote:


Hi David,

This is not a review for the posix code.

Your webrev looks good to me and corresponds to what I expected
to see. I understand that not all operating systems / platforms
are expected to have the nano second precision, so your test
probably can't go much beyond what is currently being tested.

Last time I upgraded the system clock to micro second precision [1]
I had to write a CSR and release notes. That was the first time
the clock went beyond millisecond precision however - and my
expectation is that your proposed change should no longer
require a CSR as potential nanosecond precision should
now be covered by the spec.

I had to modify a few other places as well (see [1]) - where precision
greater than 1ms was not expected, but these modifications
should cover your current changes too, so I don't think anything
more is required. Some --test-repeat might be in order
to verify things are stable but I don't expect any issue there :-)

LGTM.

best regards,

-- daniel

[1] https://bugs.openjdk.java.net/browse/JDK-8068730

On 26/05/2020 05:59, David Holmes wrote:
 > bug: https://bugs.openjdk.java.net/browse/JDK-8242504
 > webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
 >
 > 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

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

[David Holmes]

Hi Daniel,

Thanks for the review on the API side, and for the detailed 
consideration of any potential spec issues - of which they are none I'm 
glad to say.


Cheers,
David

On 27/05/2020 1:35 am, Daniel Fuchs wrote:

Hi David,

This is not a review for the posix code.

Your webrev looks good to me and corresponds to what I expected
to see. I understand that not all operating systems / platforms
are expected to have the nano second precision, so your test
probably can't go much beyond what is currently being tested.

Last time I upgraded the system clock to micro second precision [1]
I had to write a CSR and release notes. That was the first time
the clock went beyond millisecond precision however - and my
expectation is that your proposed change should no longer
require a CSR as potential nanosecond precision should
now be covered by the spec.

I had to modify a few other places as well (see [1]) - where precision
greater than 1ms was not expected, but these modifications
should cover your current changes too, so I don't think anything
more is required. Some --test-repeat might be in order
to verify things are stable but I don't expect any issue there :-)

LGTM.

best regards,

-- daniel

[1] https://bugs.openjdk.java.net/browse/JDK-8068730

On 26/05/2020 05:59, David Holmes wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

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

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

[David Holmes]

Thanks Roger!

David

On 27/05/2020 12:28 am, Roger Riggs wrote:

Looks good.

Thanks to Mark and you for the improvement and testing.



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/

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

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

[Mark Kralj-Taylor]

David,
Thanks for taking this enhancement, and making it work on the older glibc
(pre 2.17) Linux platforms currently supported by openjdk.

I like that it is a small change to split the JVM startup check on
availability of Posix clock_gettime/getres() APIs and then if additionally
CLOCK_MONOTONIC is supported.

On Daniel's comment on the spec. Yes the nice part of this is that the
JavaDoc on Clock.systemUTC() is worded to allow higher clock resolution
than system.currentTimeMillis().

Please let me know if I can help any more on this.
Mark

On Tue, 26 May 2020 at 16:35, Daniel Fuchs  wrote:

> Hi David,
>
> This is not a review for the posix code.
>
> Your webrev looks good to me and corresponds to what I expected
> to see. I understand that not all operating systems / platforms
> are expected to have the nano second precision, so your test
> probably can't go much beyond what is currently being tested.
>
> Last time I upgraded the system clock to micro second precision [1]
> I had to write a CSR and release notes. That was the first time
> the clock went beyond millisecond precision however - and my
> expectation is that your proposed change should no longer
> require a CSR as potential nanosecond precision should
> now be covered by the spec.
>
> I had to modify a few other places as well (see [1]) - where precision
> greater than 1ms was not expected, but these modifications
> should cover your current changes too, so I don't think anything
> more is required. Some --test-repeat might be in order
> to verify things are stable but I don't expect any issue there :-)
>
> LGTM.
>
> best regards,
>
> -- daniel
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8068730
>
> On 26/05/2020 05:59, David Holmes wrote:
> > bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> > webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
> >
> > 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
>
>

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

[Daniel Fuchs]

Hi David,

This is not a review for the posix code.

Your webrev looks good to me and corresponds to what I expected
to see. I understand that not all operating systems / platforms
are expected to have the nano second precision, so your test
probably can't go much beyond what is currently being tested.

Last time I upgraded the system clock to micro second precision [1]
I had to write a CSR and release notes. That was the first time
the clock went beyond millisecond precision however - and my
expectation is that your proposed change should no longer
require a CSR as potential nanosecond precision should
now be covered by the spec.

I had to modify a few other places as well (see [1]) - where precision
greater than 1ms was not expected, but these modifications
should cover your current changes too, so I don't think anything
more is required. Some --test-repeat might be in order
to verify things are stable but I don't expect any issue there :-)

LGTM.

best regards,

-- daniel

[1] https://bugs.openjdk.java.net/browse/JDK-8068730

On 26/05/2020 05:59, David Holmes wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

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

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

[Roger Riggs]

Looks good.

Thanks to Mark and you for the improvement and testing.



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/

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

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

[Stephen Colebourne]

I'm not an official OpenJDK reviewer, nor would I be confident
reviewing the native code here.
Stephen

On Tue, 26 May 2020 at 14:28, David Holmes  wrote:
>
> On 26/05/2020 6:14 pm, Stephen Colebourne wrote:
> > AFAICT a nanosecond clock is fine from a java.time.* API perspective.
>
> Thanks Stephen. Is this a review or just a nod of approval? :)
>
> Cheers,
> David
> -
>
> > Stephen
> >
> > On Tue, 26 May 2020 at 06:01, David Holmes  wrote:
> >>
> >> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> >> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
> >>
> >> 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

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

[David Holmes]

Hi Vyom,

Thanks for looking at this.

On 26/05/2020 6:44 pm, Vyom Tiwari wrote:

Hi David,

we can remove the redundant local variable(jlong result) from if block 
as follows.


return jlong(ts.tv_sec) * MILLIUNITS +  jlong(ts.tv_nsec) / 
NANOUNITS_PER_MILLIUNIT;


Sure. I copied the code from os::javaTimeNanos. IIRC I introduced the 
local to allow adding a printf before the return when debugging the 
conversion :) I should have removed it again.


Thanks,
David


Vyom


On Tue, May 26, 2020 at 10:29 AM David Holmes > wrote:


bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

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



--
Thanks,
Vyom

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

[David Holmes]

On 26/05/2020 6:14 pm, Stephen Colebourne wrote:

AFAICT a nanosecond clock is fine from a java.time.* API perspective.


Thanks Stephen. Is this a review or just a nod of approval? :)

Cheers,
David
-


Stephen

On Tue, 26 May 2020 at 06:01, David Holmes  wrote:


bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

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

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

[Vyom Tiwari]

Hi David,

we can remove the redundant local variable(jlong result) from if block as
follows.

return jlong(ts.tv_sec) * MILLIUNITS + jlong(ts.tv_nsec) /
NANOUNITS_PER_MILLIUNIT;

Vyom


On Tue, May 26, 2020 at 10:29 AM David Holmes 
wrote:

> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
>
> 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
>


-- 
Thanks,
Vyom

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

[Stephen Colebourne]

AFAICT a nanosecond clock is fine from a java.time.* API perspective.
Stephen

On Tue, 26 May 2020 at 06:01, David Holmes  wrote:
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
>
> 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

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

[David Holmes]

bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

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