RFR (S): 8211175: Remove temporary clock initialization duplication

[David Holmes]

Hi Mikael,

Thanks for looking at this.

On 3/10/2018 4:44 AM, Mikael Vidstedt wrote:
> 
> Great cleanup, thanks for doing this! Looks good.
> 
> One question:
> 
> If I read this correctly _clock_gettime and _clock_getres are always set 
> up together - they’re either both NULL, or both set to a valid function? 
> (With this change the exception to that seems to be BSD (but not 
> OSX/APPLE), but AFAICT the BSD code could/should also use the posix code.)
> 
> Asuming they’re both set up, should this (new code):
> 
>     if (pthread_getcpuclockid_func &&
>         pthread_getcpuclockid_func(_main_thread, &clockid) == 0 &&
>        os::Posix::clock_getres(clockid, &tp) == 0 && tp.tv_sec == 0) {
>       _supports_fast_thread_cpu_time = true;
>       _pthread_getcpuclockid = pthread_getcpuclockid_func;
> 
> 
> Perhaps be this:
> 
>     if (pthread_getcpuclockid_func &&
>         pthread_getcpuclockid_func(_main_thread, &clockid) == 0 &&
>        os::Posix::supports_monotonic_clock() &&  // Added check here

Technically supports_monotonic_clock means both clock_gettime is 
available and that clock_gettime(CLOCK_MONOTONIC, ts) appears to work 
correctly. In principle you could have a bad CLOCK_MONOTONIC but working 
pthread_getcpuclockid() functionality.

>        os::Posix::clock_getres(clockid, &tp) && // No need to check return value here

You do need to check the return value in case there is an error. Granted 
the only error should be an invalid clock_id which should not be 
possible at this time, but better safe than sorry.

>        tp.tv_sec == 0) {
> 
>       _supports_fast_thread_cpu_time = true;
> 
>       _pthread_getcpuclockid = pthread_getcpuclockid_func;
> 
> 
> With that change, as far as I can tell there are no calls 
> to os::Posix::clock_gettime or os::Posix::clock_getres which do *not* 
> expect to get back status == 0, in which case the code in clock_gettime 
> and clock_getres code could be rewritten to be an assert or guarantee 
> instead, and never return -1 failure - they could even be void return 
> functions. Thoughts?

In general you should still be checking the return code of clock_gettime 
and clock_getres for a possible error. Granted the only potential error 
is an invalid clock_id which normally shouldn't be possible. That said 
we also got bitten by a similar issue in  JDK-8205878 
"pthread_getcpuclockid is expected to return 0 code ".

There should be a more consistent policy applied to checking return 
values of these functions but that's outside the scope of this cleanup.

Thanks,
David

> Cheers,
> Mikael
> 
>> On Oct 1, 2018, at 11:08 PM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> webrev: http://cr.openjdk.java.net/~dholmes/8211175/webrev/ 
>> <http://cr.openjdk.java.net/%7Edholmes/8211175/webrev/>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8211175
>>
>> This cleans up some code duplication between os::Linux and os::Posix. 
>> os::Posix now exports the necessary clock functions so the Linux code 
>> can use those.
>>
>> Also cleaned up the clock_getres syscall as that is already provided 
>> via the direct clock_getres (no need for a syscall).
>>
>> Finally, as code in os::Linux already assumed clock_gettime etc was 
>> always available at build-time (and run-time for that matter) this is 
>> now explicitly checked via:
>>
>> + #ifndef SUPPORTS_CLOCK_MONOTONIC
>> + #error "Build platform doesn't support clock_gettime and related 
>> functionality"
>> + #endif
>>
>> Testing: tiers 1-3 (mach5)
>>
>> Thanks,
>> David
>