OpenLightingProject
Replace gettimeofday() with clock_gettime() where possible
See https://blog.habets.se/2010/09/gettimeofday-should-never-be-used-to-measure-time
and
http://stackoverflow.com/questions/11680461/monotonic-clock-on-osx
Hi there, found your project through Hacktoberfest. Could I take a look at this issue?
Yes @stefansiegfried , please feel free!
It looks like we use our wrapper function CurrentTime in quite a lot of places, a handful just to see the wall-clock time, but mostly to see how much time has elapsed (i.e. the naughty version).
If you've not found it yet, our Doxygen docs will probably be useful for this: http://docs.openlighting.org/ola/doc/latest/
I suspect as a start, and realising this actually looks like it could be a simple, but quite involved PR (there's lots of places it's used in the code) adding some stuff to detect when the monotonic clock is available and then probably having a CurrentMonotonicTime or something function which falls back to CurrentTime, then tweaking http://docs.openlighting.org/ola/doc/latest/classola_1_1_time_interval.html to do some offset calculations could make sense, but I'm welcome to other ideas too.
Please get in touch if you've got any more comments.
Thanks, @peternewman. I was able to compile the project's 'make all' target, however I have run into some trouble with 'make check' unit tests and g++ errors (-Werror=deprecated-copy). Where is the best place troubleshoot that? Should I open a separate ticket, or can we discuss that here?
Ah, sorry to hear that. I'd suggest raising a new issue for that is probably best, as it may affect others too.
When you do so, can you let us know OS, version etc. In the meantime, adding --disable-fatal-warnings to configure may allow you to continue.
After doing some reading, I do have some questions on implementation:
-
Is there any concern with CLOCK_MONOTONIC not advancing during suspend?
-
Is the suggestion to replace all "naughty" calls to CurrentTime with CurrentMonotonicTime (which then falls back to CurrentTime)?
-
The clock_gettime function accepts a struct timespec as a parameter versus gettimeofday which takes a struct timeval. Would you suggest modifying BaseTimeVal/TimeStamp/TimeInterval to also handle struct timespec?
Or, replace gettimeofday with clock_gettime(CLOCK_REALTIME) and just use timespec instead of timeval? Regarding gettimeofday, GNU documentation at https://www.gnu.org/software/libc/manual/html_node/Getting-the-Time.html states "Portability Note: As of the 2008 revision of POSIX, this function is considered obsolete. The GNU C Library will continue to provide this function indefinitely, but new programs should use clock_gettime instead."
-
I think one issue with replacing gettimeofday is that older versions of OS X, prior to 10.12, do not support clock_gettime according to posts at https://stackoverflow.com/questions/5167269/clock-gettime-alternative-in-mac-os-x. Is there a requirement for this project to support those older OS X versions?
-
Are there any issues with using preprocessor directives to solve this problem? e.g. to detect the target OS: #ifdef MACH ...
1. Is there any concern with CLOCK_MONOTONIC not advancing during suspend?
Possibly, most of our uses are things like sending hearbeats every few seconds or similar, if the machine is suspended we're unlikely to hit most of those anyway so will appear offline (as we will be). Whereas if the clock changes due to NTP for example, we're still present the whole time so this fix should ensure we send a heartbeat every two seconds rather than every two wallclock seconds.
2. Is the suggestion to replace all "naughty" calls to CurrentTime with CurrentMonotonicTime (which then falls back to CurrentTime)?
I think that's the simple solution, assuming you mean CurrentMonotonicTime falls back internally. I think if you look at the code internally, most are doing the calculations too, so you could perhaps look at a wrapper function that effectively tells you if a TimeInterval has expired (or calculates a TimeInterval) which then uses CurrentMonotonicTime internally, but that could be a later extension.
3. The clock_gettime function accepts a struct timespec as a parameter versus gettimeofday which takes a struct timeval. Would you suggest modifying BaseTimeVal/TimeStamp/TimeInterval to also handle struct timespec?
Probably a bit up to you given it will be wrapped up anyway, whether you extend the Time objects or the wrapper call.
Or, replace gettimeofday with clock_gettime(CLOCK_REALTIME) and just use timespec instead of timeval? Regarding gettimeofday, GNU documentation at https://www.gnu.org/software/libc/manual/html_node/Getting-the-Time.html states "Portability Note: As of the 2008 revision of POSIX, this function is considered obsolete. The GNU C Library will continue to provide this function indefinitely, but new programs should use clock_gettime instead."
In which case perhaps try and move over where it's available.
1. I think one issue with replacing gettimeofday is that older versions of OS X, prior to 10.12, do not support clock_gettime according to posts at https://stackoverflow.com/questions/5167269/clock-gettime-alternative-in-mac-os-x. Is there a requirement for this project to support those older OS X versions?
Potentially, or at least I'd rather not drop it if we can just add some wrappers. We also compile on various other random platforms including a number of BSD variants, so they may have similar compatibility.
2. Are there any issues with using preprocessor directives to solve this problem? e.g. to detect the target OS: #ifdef **MACH** ...
No, we do similar elsewhere already. If you have a nose through the code (e.g. the network stuff) you'll find various examples. If this is likely to impact a number of places (which I suspect it may with the Time objects too), we sometimes set a define within configure and then use that throughout, rather than having a large amount of duplicate logic in lots of places (e.g. see some of the libusb stuff).
We should check the Java and Python codebase for the same stuff before we actually close this.
See discussion here: https://stackoverflow.com/questions/14248033/clock-monotonic-and-pthread-mutex-timedlock-pthread-cond-timedwait
We could fix the pthread calls, but we'd need to make sure both sides are in sync which shouldn't be too hard. One for a later PR though given I guess technically pthread_condattr_setclock may not exist on some OSes (see for example this chaos https://github.com/OpenLightingProject/ola/blob/master/config/ac_pthread_set_name.m4 ).
@stefansiegfried has done the bulk of the C++ apart from the pthread stuff mentioned above in #1673 now.
Todo:
- [x] Change main C++ code
- [ ] Change pthread C++ code
- [ ] Check/change Python code
- [ ] Check/change Java code
Do you want to have a think at some point if you fancy attempting some or all of the other bits here and let me know, otherwise I'll unassign it from you.
Do you want to have a think at some point if you fancy attempting some or all of the other bits here and let me know, otherwise I'll unassign it from you.
It's probably best to unassign from me for now in case someone else wants to pick it up.
It's probably best to unassign from me for now in case someone else wants to pick it up.
Okay no worries, thanks again for all your efforts. I suspect it will still be floating around in the future if you want to pick it up again.
See https://github.com/OpenLightingProject/ola/issues/779#issuecomment-719817449 for outstanding bits.
So there's also a CLOCK_MONOTONIC_RAW which sounds like it might be better where available: https://man7.org/linux/man-pages/man2/clock_getres.2.html