sys: util: timeutil: add utils for manipulating struct timespec #90060
Conversation
| - timeutils | ||
| integration_platforms: | ||
| - native_sim/64 | ||
| tests: |
There was a problem hiding this comment.
It would also be good to add a testcase to verify the non-builtin code path.
Keeping this in draft until I've done that.
|
cc @trembel |
There was a problem hiding this comment.
Since timespec is specific to posix, should it be moved to include/zephyr/posix/ where
struct` timespec {
time_t tv_sec;
long tv_nsec;
};
is also defined? include/zephyr/sys I believe is primarily for zephyr native libraries, though there obviously are a few overlaps there like fdtable. Not sure how or where we draw such lines :)
include/zephyr/sys/timespec_util.h
Outdated
| * | ||
| * @return `true` if the operation would result in integer overflow, otherwise `false`. | ||
| */ | ||
| static inline bool timespec_add(struct timespec *a, const struct timespec *b) |
There was a problem hiding this comment.
if timespec itself is not nameclashing, the header should probably just be named timespec.h given the namespace timespec_util is not used for any function.
There was a problem hiding this comment.
I just merged it directly into timeutil.h
include/zephyr/sys/timespec_util.h
Outdated
There was a problem hiding this comment.
Based on the structure of the documentation, it might be better just to add the contents of this file directly to include/zephyr/sys/timeutil.h and adjust doxygen groups accordingly.
cfriedt
changed the title
There was a problem hiding this comment.
A few comments:
First, the bool return value. It feels to me like it should be the other
way around i.e. true=OK and false=problem. Currently you have things like:
return timespec_negate(&neg) || timespec_add(a, &neg);
This just looks semantically wrong. It should be like: if we can negate
and add then return OK e.g.:
return timespec_negate(&neg) && timespec_add(a, &neg);
Another example:
return __builtin_add_overflow(a->tv_sec, b->tv_sec, &a->tv_sec) ||
__builtin_add_overflow(a->tv_nsec, b->tv_nsec, &a->tv_nsec) ||
timespec_normalize(a);
IMHO this would be more logically readable as:
return !__builtin_add_overflow(a->tv_sec, b->tv_sec, &a->tv_sec) &&
!__builtin_add_overflow(a->tv_nsec, b->tv_nsec, &a->tv_nsec) &&
timespec_normalize(a);
Translation: if no overflow on the first add and no overflow on the second
add and normalization worked then return OK (true). Yes,
__builtin_add_overflow() returns true on error but that's in the name.
Here we have timespec_add() and the name doesn't imply an error.
Users would have more readable code if:
if (timespec_add(a, b) {
/* true: the addition was possible */
} else {
/* false: addition was _not_ possible */
}
Next, I'm a little concerned by the optimization. This:
int64_t sec = (ts->tv_nsec >= NSEC_PER_SEC) * (ts->tv_nsec / NSEC_PER_SEC) +
+ (ts->tv_nsec < 0) * DIV_ROUND_UP(-ts->tv_nsec, NSEC_PER_SEC);
That's potentially two divisions just for the purpose of avoiding a branch.
On many architectures (if not most) a branch is still cheaper than a
division, even more so if the division ends up in a library call given
a 64-bit dividend.
And finally, you should add more values to your test case. In particular,
tests with tv_nsec being -NSEC_PER_SEC - 1, -2*NSEC_PER_SEC + 1,
-2*NSEC_PER_SEC - 1, etc. would provide more coverage.
@bjarki-andreasen - mentioned in these docs as well as the C standard, while |
Fair enough :) I have no strong opinion, just noticed that we do define the timespec struct in the posix folder. Anyway PR looks good, definate improvement! |
From my perspective, that is worse - there are 5 logical operations instead of 3.
I'm sort of thinking I would prefer to just append
Yeah, I could definitely rename that to https://github.com/zephyrproject-rtos/zephyr/tree/main/include/zephyr/sys/math_extras.h
Agreed. What I initially had is only cheap on processors that have e.g. icache, dcache, HW divide, and only cheap compared to potential cache misses. I would be happy to use
Absolutely - thanks for the feedback @npitre. |
swap the relative position of the __cplusplus closing bracket and the doxygen end-group comment. Signed-off-by: Chris Friedt <[email protected]>
Yeah, that's true - we should probably eliminate duplicate definitions especially with #30105 One kind of pervasive thing in POSIX (not sure if it's like this in the c spec as much) is that types can be originally defined in several header files. E.g. |
cfriedt
force-pushed
the
timespec-utils
branch
2 times, most recently
from
e72e78a to
7aeb687
Compare
There was a problem hiding this comment.
IMHO this would be more logically readable as:
return !__builtin_add_overflow(a->tv_sec, b->tv_sec, &a->tv_sec) && !__builtin_add_overflow(a->tv_nsec, b->tv_nsec, &a->tv_nsec) && timespec_normalize(a);From my perspective, that is worse - there are 5 logical operations
instead of 3.
Absolutely not. There are no more operations. The compiler is happy
to flip the logic around as it see fit and it won't care one way or the
other.. OTOH this is better semantics for humans reading the code.
I'm sort of thinking I would prefer to just append
_overflow. To me,
it's less useful to add multiple extra logical operations.
Please don't! This is an abomination!
Adding _overflow everywhere instead of ! in a few places is a really
a bad tradeoff. Again, the compiler doesn't care either ways (the code won't
be any bigger or any less efficient).
Add a number of utility functions for manipulating struct timespec. * timespec_add_overflow() * timespec_compare() * timespec_equal() * timespec_is_valid() * timespec_negate_overflow() * timespec_normalize_overflow() * timespec_sub_overflow() If the `__builtin_add_overflow()` and `__builtin_sub_overflow()` functions are available, then these functions use are mostly branchless, which should provide decent performance on systems with an instruction cache. Otherwise, manually written alternatives exist that are also perhaps more readable. Signed-off-by: Chris Friedt <[email protected]>
cfriedt
force-pushed
the
timespec-utils
branch
2 times, most recently
from
c2ef0d4 to
8816044
Compare
Add a timespec util testsuite. This should have reasonably high enough coverage to be useful. I would have preferred to add this as an architecture-independent unit test (for the unit_testing platform) under tests/unit/timeutil but there is an inconsistency about the size of time_t on the unit_testing and native_sim/native platforms. On every other platform supported by Zephyr, time_t is 64-bits. However, on those platforms, time_t is only 32-bits. Signed-off-by: Chris Friedt <[email protected]>
Use the newly added timespec util functions to manipulate and compare timespec structures with overflow detection. Signed-off-by: Chris Friedt <[email protected]>
Add documentation in the timeutil group for recently added functions for validating, comparing, and manipulating `struct timespec` objects. Signed-off-by: Chris Friedt <[email protected]>
Hehe.. sure, if you insist. I feel less strongly |
|
Add a number of utility functions for manipulating struct timespec.
If the
__builtin_add_overflow()and__builtin_sub_overflow()functions are available, then these functions use are mostly branchless, which should provide decent performance on systems with an instruction cache. Otherwise, manually written alternatives exist that are also perhaps more readable.Fixes #88115
Doc Preview
Testing Done: