RFR: 8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC [v3]

[David Holmes]

Hi Gerard,

Thanks for looking at this!

On 21/01/2021 7:34 am, Gerard Ziemski wrote:
> On Tue, 19 Jan 2021 01:53:03 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> We are now confident that we have build-time and runtime support for clock_gettime and CLOCK_MONOTONIC on all our OpenJDK supported POSIX platforms - see bug report for some more details on different OS. Consequently we can simplify a lot of the code in this area and move common code to os_posix.
>>>
>>> As of glibc 2.17 the necessary functions are in glibc rather than librt, but we (Oracle at least) aren't yet in position to set our minimum Linux version to support that. We still have supported platforms at glibc 2.12. So to address that we link librt at build time on Linux. This seems to work find for older and more modern Linuxes and also works for the Apline Linux with Musl variant.
>>>
>>> The changes are in layered commits:
>>>
>>> Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true and removed.
>>> Step 2: make supports_monotonic_clock always true and so remove checking in OS code
>>> Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME)
>>> Step 4: Move shared time functions to os_posix.cpp
>>> Step 5: Alway link librt on Linux so we don't rely on glibc > 2.17
> 
> src/hotspot/os/bsd/os_bsd.inline.hpp line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 1999, 2030, Oracle and/or its affiliates. All rights reserved.
> 
> Year 2030 ?

No idea how that happened :) Fixed via merge with mainline.

> src/hotspot/os/bsd/os_bsd.cpp line 828:
> 
>> 826: }
>> 827:
>> 828: void os::javaTimeNanos_info(jvmtiTimerInfo *info_ptr) {
> 
> How come we need `os::javaTimeNanos_info` here if there is already one in `os_posix.cpp`?

javaTimeNanos_info describes the properties of the implementation of 
javaTimeNanos. As we have a custom implementation of javaTimeNanos_info 
for macOS (and AIX) it seemed appropriate to also define 
javaTimeNanos_info, even if it happens to be the same as the Posix 
version. Alternatively I could put a comment block in there warning 
anyone who might change the implementation to check if the Posix version 
is still correct (and I'd change the ifdefs in the os_posix.cpp code).

> src/hotspot/os/posix/os_posix.hpp line 95:
> 
>> 93:   static void    ucontext_set_pc(ucontext_t* ctx, address pc);
>> 94:
>> 95:   static bool supports_monotonic_clock();
> 
> Why do we need this API at all now?
> 
> thread.cpp uses it here:
> 
>        assert(!os::supports_monotonic_clock(),
>               "unexpected time moving backwards detected in JavaThread::sleep()");
> 
> so we can just remove this usage?

Good catch! Yes we can do this. I had intended to do it but got 
distracted by the linking issues and forgot about it.

> src/hotspot/os/posix/os_posix.cpp line 1161:
> 
>> 1159:
>> 1160: void os::Posix::init_2(void) {
>> 1161:   log_info(os)("Use of CLOCK_MONOTONIC is supported");
> 
> We are keeping this output to avoid breaking whomever might be looking for this text?

I find it useful information, especially in conjunction with the lines 
that follow, and allows comparing different JDK versions.

Thanks,
David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/2090
>