Restore ability to use clock_gettime() on posix systems #3168
Conversation
|
arf ...
|
|
Still this issue : Not sure if it's really possible to force a specific posix level. |
- cmake/meson pass -std=gnu99 which hits clock() fallback - clock() fallback skews multi-threading benchmarks - TIME_UTC (C11) is similar to CLOCK_REALTIME (POSIX) - GNU may hide CLOCK_REALTIME in -std=c99 (__STRICT_ANSI__) - Switch to CLOCK_MONOTONIC like before 36d2dfd
which restore clock_gettime() as a way to get a usable timer for posix systems. Original PR by @jbeich
|
Interestingly, if I comment out This can't be good. |
also : ensure that platform.h is included first, before any standard library. Note : this is fairly fragile, any future change to include order can break this pattern. And there is no automated test to detect this.
|
This is actually a very problematic issue. I think I fixed it, ensuring that but that's fragile. In the future, any contributor may change or update the #include order in one of the affected files, this will probably be silent and seem harmless, but would then result in a sibtle change of interface interpretation, resulting in different |
| @@ -95,10 +92,19 @@ UTIL_time_t UTIL_getTime(void) | |||
| /* time must be initialized, othersize it may fail msan test. | |||
| * No good reason, likely a limitation of timespec_get() for some target */ | |||
| UTIL_time_t time = UTIL_TIME_INITIALIZER; | |||
| #if defined(TIMEFN_HAS_TIMESPEC_GET) | |||
| /* favor timespec_get() as first choice for c11 targets */ | |||
There was a problem hiding this comment.
For testing: could we add something like asm("\n# Using timespec_get") and asm("\n# Using clock_gettime") to each branch of the ifdef? Then we could grep the generated assembly for those strings and ensure only one string is included.
(I'm not sure exactly how to insert a comment or symbol into the assembly, took the above code from https://stackoverflow.com/a/28493396/3154996. I think it is definitely possible, though).
There was a problem hiding this comment.
That would add some code specifically for this test.
It would have to be enabled through some dedicated setting (like a macro) since we don't want that for the general case.
Then indeed it would be possible to couple that generation with an external tool that grep the outcome and ensure it's consistent. This asm("# message") thing seems to only leave a comment in the generated assembly. So that means accessing the generated assembly and grep it.
That's some work, but it definitely looks plausible.
There was a problem hiding this comment.
It seems like we can get text into the actual binary like this: asm volatile(".global message \n message:") (godbolt). I can test if this works for our situation and if so put up a PR which does it (gated to DEBUGLEVEL >= 2 or something). Agree that getting the generated assembly would require some work, hopefully that's not necessary.
Edit: it might be a problem for linking if it is inserted in multiple places, if so I can add a macro specifically for this test.
There was a problem hiding this comment.
Yes, this would be a welcome test capability.
As it stands, this PR is stuck as long as there is no way to reliably test timefn interface and ensure it's interpreted the same way in all units that include it.
|
Another possibility : This is obviously more work, and requires refactoring |
|
Replaced by #3423 |
When
timespec_get()was unavailable,timefnunit would fall back toclock_t,which is unfortunately not good enough to measure speed in multi-threading scenarios.
@jbeich proposes a patch which re-enable
clock_gettime()as an alternative for posix systems.(I can't remember why it was removed...)
This extends the nb of platforms that can use a timer function compatible with multi-threading scenarios.