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

[Mikael Vidstedt]

That all makes sense, thanks for the additional information and agree that it’s out of scope for the change in either case!

Cheers,
Mikael

> On Oct 2, 2018, at 1:32 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> 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