Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor timefn, restore support for clock_gettime() #3423

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Refactor timefn, restore support for clock_gettime() #3423

merged 3 commits into from
Jan 18, 2023

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Jan 13, 2023

This replaces #3168

An issue experienced last year was that posix systems without access to timespec_get() (C11) fall back to C90 clock(), which is a low resolution timer, actually indexed on process time, and therefore unable to properly measure time when there is a multi-threaded workload. This led to some confusion over performance numbers last year.

Since then, we have added a warning message, and now when benchmarking in multi-threading mode with only the clock_t low resolution timer, users get warned that the measurement is probably incorrect.

Another proposal to improve this situation was to restore support of clock_gettime() high resolution timer, which would help all POSIX 2001 compliant systems.
Unfortunately, the current interface of timefn.h leads to potential confusion on this point, because the state of POSIX macros depends on #include order, which is brittle. This results in situations where timefn.h interface is interpreted differently depending on local #include order, resulting in different objects being allocated on stack.

This PR changes the interface logic, so that the exposed time_t object is now solely define by timefn.h and completely independent of OS peculiarity (suggestion by @terrelln). Adaptation to the local OS high resolution timer happens solely within timefn.c. Therefore, users of timefn.h no longer risk interpreting the interface differently.

This makes it possible to re-enable clock_gettime() (also done in this PR).

Follow-up 1 : during tests, I employed c89build target to test the low resolution time. It discovered a number of places with non-C90 compliant syntax (// line comments mostly, and one inline). This tells me that the c89build test is not played in CI. It should probably be added.

Follow-up 2 : the symbols in timefn.h are prefixed UTIL_*, which is oddly unspecific.
I was considering to rename all symbols of this unit, typically employing the TIME_* prefix instead.

The timer storage type is no longer dependent on OS.
This will make it possible to re-enable posix precise timers
since the timer storage type will no longer be sensible to #include order.
See #3168 for details of pbs of previous interface.

Suggestion by @terrelln
This should notably allow posix systems with timespec_get()
to have access to a high resolution timer,
instead of falling back to C90's clock_t.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants