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

Mikael Vidstedt mikael.vidstedt at oracle.com
Tue Oct 2 20:45:07 UTC 2018

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 clockgettime and clockgetres 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 (pthreadgetcpuclockidfunc && pthreadgetcpuclockidfunc(mainthread, &clockid) == 0 && os::Posix::clockgetres(clockid, &tp) == 0 && tp.tvsec == 0) { supportsfastthreadcputime = true; pthreadgetcpuclockid = pthreadgetcpuclockidfunc; Perhaps be this: if (pthreadgetcpuclockidfunc && pthreadgetcpuclockidfunc(mainthread, &clockid) == 0 && os::Posix::supportsmonotonicclock() && // Added check here Technically supportsmonotonicclock means both clockgettime is available and that clockgettime(CLOCKMONOTONIC, ts) appears to work correctly. In principle you could have a bad CLOCKMONOTONIC but working pthreadgetcpuclockid() functionality.

os::Posix::clockgetres(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 clockid which should not be possible at this time, but better safe than sorry. tp.tvsec == 0) { supportsfastthreadcputime = true; pthreadgetcpuclockid = pthreadgetcpuclockidfunc; With that change, as far as I can tell there are no calls to os::Posix::clockgettime or os::Posix::clockgetres which do not expect to get back status == 0, in which case the code in clockgettime and clockgetres 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 clockgettime and clockgetres for a possible error. Granted the only potential error is an invalid clockid which normally shouldn't be possible. That said we also got bitten by a similar issue in JDK-8205878 "pthreadgetcpuclockid 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 clockgetres syscall as that is already provided via the direct clockgetres (no need for a syscall). Finally, as code in os::Linux already assumed clockgettime etc was always available at build-time (and run-time for that matter) this is now explicitly checked via: + #ifndef SUPPORTSCLOCKMONOTONIC + #error "Build platform doesn't support clockgettime and related functionality" + #endif Testing: tiers 1-3 (mach5) Thanks, David

More information about the hotspot-runtime-dev mailing list