zstd
zstd copied to clipboard
facebook
Restore ability to use clock_gettime() on posix systems
When timespec_get() was unavailable,
timefn unit would fall back to clock_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.
arf ...
If the defined operator appears as a result of a macro expansion, the C standard says the behavior is undefined
Still this issue :
../programs/timefn.h:22: error: "_POSIX_C_SOURCE" redefined [-Werror]
22 | #define _POSIX_C_SOURCE 200809L /* clock_gettime */
Not sure if it's really possible to force a specific posix level.
This happens in the c99 test, which might force a specific level of its own.
Interestingly, if I comment out _POSIX_C_SOURCE,
-std=c99 compilation mode works fine,
but as a consequence, I see a mixture of clock_t and CLOCK_MONOTONIC,
depending on which unit includes timefn.h.
In file included from benchfn.c:20: │·
timefn.h:80:6: warning: #warning "uses C90 clock_t" [-Wcpp] │·
80 | #warning "uses C90 clock_t" │·
| ^~~~~~~ │·
CC obj/conf_63ad40fbee40d6d23986334cfd96df82/dibio.o │·
CC obj/conf_63ad40fbee40d6d23986334cfd96df82/fileio.o │·
In file included from benchzstd.c:31: │·
timefn.h:73:6: warning: #warning "uses CLOCK_MONOTONIC" [-Wcpp] │·
73 | #warning "uses CLOCK_MONOTONIC" │·
| ^~~~~~~ │·
CC obj/conf_63ad40fbee40d6d23986334cfd96df82/fileio_asyncio.o │·
CC obj/conf_63ad40fbee40d6d23986334cfd96df82/timefn.o │·
In file included from dibio.c:31: │·
timefn.h:73:6: warning: #warning "uses CLOCK_MONOTONIC" [-Wcpp] │·
73 | #warning "uses CLOCK_MONOTONIC" │·
| ^~~~~~~ │·
In file included from fileio.c:36: │·
timefn.h:73:6: warning: #warning "uses CLOCK_MONOTONIC" [-Wcpp] │·
73 | #warning "uses CLOCK_MONOTONIC" │·
| ^~~~~~~ │·
CC obj/conf_63ad40fbee40d6d23986334cfd96df82/util.o │·
CC obj/conf_63ad40fbee40d6d23986334cfd96df82/zstdcli.o │·
In file included from timefn.c:13: │·
timefn.h:80:6: warning: #warning "uses C90 clock_t" [-Wcpp] │·
80 | #warning "uses C90 clock_t" │·
| ^~~~~~~ │·
CC obj/conf_63ad40fbee40d6d23986334cfd96df82/zstdcli_trace.o │·
In file included from fileio_common.h:21, │·
from fileio_asyncio.c:23: │·
timefn.h:73:6: warning: #warning "uses CLOCK_MONOTONIC" [-Wcpp] │·
73 | #warning "uses CLOCK_MONOTONIC" │·
| ^~~~~~~ │·
In file included from zstdcli_trace.c:16: │·
timefn.h:80:6: warning: #warning "uses C90 clock_t" [-Wcpp] │·
80 | #warning "uses C90 clock_t" │·
| ^~~~~~~
This can't be good.
The unit is necessarily compiled once, with one mode, or the other, not both.
I also noticed wide measurement variations, nowhere near as stable as timespec_get() nor clock_t only.
This might be a reason.
This is actually a very problematic issue.
Having _POSIX_C_SOURCE defined differently depending on #include pattern, resulting in different interpretation of timefn.h interface, is a big source of trouble.
I think I fixed it, ensuring that _POSIX_C_SOURCE is always set through platform.h, which is itself always included first, before any standard library,
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 typedef definition, resulting in wrong objects, at best perceived through wrong time measurements...
Worse, there is no automated test to detect such discrepancy.
Another possibility :
change the timefn API in order to use opaque types,
so that the type interpretation is done solely within timefn.c,
and can't be different on user side depending on their #include order.
This is obviously more work, and requires refactoring timefn API as well as all existing calling sites.
Once completed though, it will eliminate this fragility on interpreting the return type the same way from all call sites.