From bcfb7ad03c47d3018eeaec2c68be9d37dc114151 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Thu, 12 Jan 2023 19:00:27 -0800 Subject: [PATCH] refactor timefn 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 --- programs/benchzstd.c | 7 +--- programs/dibio.c | 9 ++-- programs/fileio_common.h | 2 +- programs/timefn.c | 79 ++++++++++++++++++++---------------- programs/timefn.h | 52 +++++++----------------- tests/decodecorpus.c | 27 +++++++----- tests/external_matchfinder.h | 2 +- tests/fuzzer.c | 22 +++++----- 8 files changed, 94 insertions(+), 106 deletions(-) diff --git a/programs/benchzstd.c b/programs/benchzstd.c index 285e401ef..7770dd2a5 100644 --- a/programs/benchzstd.c +++ b/programs/benchzstd.c @@ -387,12 +387,9 @@ BMK_benchMemAdvancedNoAlloc( RDG_genBuffer(compressedBuffer, maxCompressedSize, 0.10, 0.50, 1); } -#if defined(UTIL_TIME_USES_C90_CLOCK) - if (adv->nbWorkers > 1) { - OUTPUTLEVEL(2, "Warning : time measurements restricted to C90 clock_t. \n") - OUTPUTLEVEL(2, "Warning : using C90 clock_t leads to incorrect measurements in multithreading mode. \n") + if (!UTIL_support_MT_measurements() && adv->nbWorkers > 1) { + OUTPUTLEVEL(2, "Warning : time measurements may be incorrect in multithreading mode... \n") } -#endif /* Bench */ { U64 const crcOrig = (adv->mode == BMK_decodeOnly) ? 0 : XXH64(srcBuffer, srcSize, 0); diff --git a/programs/dibio.c b/programs/dibio.c index b21338cd8..26ebe5ca1 100644 --- a/programs/dibio.c +++ b/programs/dibio.c @@ -274,21 +274,20 @@ static fileStats DiB_fileStats(const char** fileNamesTable, int nbFiles, size_t int n; memset(&fs, 0, sizeof(fs)); - // We assume that if chunking is requested, the chunk size is < SAMPLESIZE_MAX + /* We assume that if chunking is requested, the chunk size is < SAMPLESIZE_MAX */ assert( chunkSize <= SAMPLESIZE_MAX ); for (n=0; n 0) - { - // TODO: is there a minimum sample size? Can we have a 1-byte sample? + if (chunkSize > 0) { + /* TODO: is there a minimum sample size? Can we have a 1-byte sample? */ fs.nbSamples += (int)((fileSize + chunkSize-1) / chunkSize); fs.totalSizeToLoad += fileSize; } diff --git a/programs/fileio_common.h b/programs/fileio_common.h index 827a5a06b..55491b8e3 100644 --- a/programs/fileio_common.h +++ b/programs/fileio_common.h @@ -122,4 +122,4 @@ extern UTIL_time_t g_displayClock; #if defined (__cplusplus) } #endif -#endif //ZSTD_FILEIO_COMMON_H +#endif /* ZSTD_FILEIO_COMMON_H */ diff --git a/programs/timefn.c b/programs/timefn.c index 08aa1cfcb..023e2136c 100644 --- a/programs/timefn.c +++ b/programs/timefn.c @@ -13,6 +13,7 @@ #include "timefn.h" +#include /* TIME_UTC, then struct timespec and clock_t */ /*-**************************************** * Time functions @@ -20,12 +21,11 @@ #if defined(_WIN32) /* Windows */ +#include /* LARGE_INTEGER */ #include /* abort */ #include /* perror */ -UTIL_time_t UTIL_getTime(void) { UTIL_time_t x; QueryPerformanceCounter(&x); return x; } - -PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd) +UTIL_time_t UTIL_getTime(void) { static LARGE_INTEGER ticksPerSecond; static int init = 0; @@ -36,16 +36,18 @@ PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd) } init = 1; } - return 1000000000ULL*(clockEnd.QuadPart - clockStart.QuadPart)/ticksPerSecond.QuadPart; + { UTIL_time_t r; + r.t = (PTime)(QueryPerformanceCounter(&x).QuadPart * 1000000000ULL / ticksPerSecond.QuadPart); + return ; + } } - #elif defined(__APPLE__) && defined(__MACH__) -UTIL_time_t UTIL_getTime(void) { return mach_absolute_time(); } +#include /* mach_timebase_info_data_t, mach_timebase_info, mach_absolute_time */ -PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd) +UTIL_time_t UTIL_getTime(void) { static mach_timebase_info_data_t rate; static int init = 0; @@ -53,12 +55,18 @@ PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd) mach_timebase_info(&rate); init = 1; } - return ((clockEnd - clockStart) * (PTime)rate.numer) / ((PTime)rate.denom); + { UTIL_time_t r; + r.t = mach_absolute_time() * (PTime)rate.numer / (PTime)rate.denom; + return r; + } } -/* C11 requires timespec_get, but FreeBSD 11 lacks it, while still claiming C11 compliance. - Android also lacks it but does define TIME_UTC. */ +/* C11 requires timespec_get(). + * However, FreeBSD 11 claims C11 compliance but lacks timespec_get(). + * Double confirm by requiring the definition of TIME_UTC. + * However, some versions of Android manage to simultanously define TIME_UTC + * and lack timespec_get() support... */ #elif (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) /* C11 */) \ && defined(TIME_UTC) && !defined(__ANDROID__) @@ -69,46 +77,38 @@ 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; + struct timespec time = { 0, 0 }; if (timespec_get(&time, TIME_UTC) != TIME_UTC) { perror("timefn::timespec_get"); abort(); } - return time; -} - -static UTIL_time_t UTIL_getSpanTime(UTIL_time_t begin, UTIL_time_t end) -{ - UTIL_time_t diff; - if (end.tv_nsec < begin.tv_nsec) { - diff.tv_sec = (end.tv_sec - 1) - begin.tv_sec; - diff.tv_nsec = (end.tv_nsec + 1000000000ULL) - begin.tv_nsec; - } else { - diff.tv_sec = end.tv_sec - begin.tv_sec; - diff.tv_nsec = end.tv_nsec - begin.tv_nsec; + { UTIL_time_t r; + r.t = (PTime)time.tv_sec * 1000000000ULL + (PTime)time.tv_nsec; + return r; } - return diff; -} - -PTime UTIL_getSpanTimeNano(UTIL_time_t begin, UTIL_time_t end) -{ - UTIL_time_t const diff = UTIL_getSpanTime(begin, end); - PTime nano = 0; - nano += 1000000000ULL * diff.tv_sec; - nano += diff.tv_nsec; - return nano; } #else /* relies on standard C90 (note : clock_t produces wrong measurements for multi-threaded workloads) */ -UTIL_time_t UTIL_getTime(void) { return clock(); } -PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd) { return 1000000000ULL * (clockEnd - clockStart) / CLOCKS_PER_SEC; } +UTIL_time_t UTIL_getTime(void) +{ + UTIL_time_t r; + r.t = (PTime)clock() * 1000000000ULL / CLOCKS_PER_SEC; + return r; +} + +#define TIME_MT_MEASUREMENTS_NOT_SUPPORTED #endif /* ==== Common functions, valid for all time API ==== */ +PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd) +{ + return clockEnd.t - clockStart.t; +} + PTime UTIL_getSpanTimeMicro(UTIL_time_t begin, UTIL_time_t end) { return UTIL_getSpanTimeNano(begin, end) / 1000ULL; @@ -134,3 +134,12 @@ void UTIL_waitForNextTick(void) clockEnd = UTIL_getTime(); } while (UTIL_getSpanTimeNano(clockStart, clockEnd) == 0); } + +int UTIL_support_MT_measurements(void) +{ +# if defined(TIME_MT_MEASUREMENTS_NOT_SUPPORTED) + return 0; +# else + return 1; +# endif +} diff --git a/programs/timefn.h b/programs/timefn.h index 3e4476a4e..b814ff8d8 100644 --- a/programs/timefn.h +++ b/programs/timefn.h @@ -18,7 +18,7 @@ extern "C" { /*-**************************************** -* Local Types +* Types ******************************************/ #if !defined (__VMS) && (defined (__cplusplus) || (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) /* C99 */) ) @@ -32,40 +32,11 @@ extern "C" { typedef unsigned long long PTime; /* does not support compilers without long long support */ #endif - - -/*-**************************************** -* Time types (note: OS dependent) -******************************************/ -#include /* TIME_UTC, then struct timespec and clock_t */ - -#if defined(_WIN32) /* Windows */ - - #include /* LARGE_INTEGER */ - typedef LARGE_INTEGER UTIL_time_t; - #define UTIL_TIME_INITIALIZER { { 0, 0 } } - -#elif defined(__APPLE__) && defined(__MACH__) - - #include - typedef PTime UTIL_time_t; - #define UTIL_TIME_INITIALIZER 0 - -/* C11 requires timespec_get, but FreeBSD 11 lacks it, while still claiming C11 compliance. - Android also lacks it but does define TIME_UTC. */ -#elif (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) /* C11 */) \ - && defined(TIME_UTC) && !defined(__ANDROID__) - - typedef struct timespec UTIL_time_t; - #define UTIL_TIME_INITIALIZER { 0, 0 } - -#else /* relies on standard C90 (note : clock_t produces wrong measurements for multi-threaded workloads) */ - - #define UTIL_TIME_USES_C90_CLOCK - typedef clock_t UTIL_time_t; - #define UTIL_TIME_INITIALIZER 0 - -#endif +/* UTIL_time_t contains a nanosecond time counter. + * The absolute value is not meaningful. + * It's only valid to compute the difference between 2 measurements. */ +typedef struct { PTime t; } UTIL_time_t; +#define UTIL_TIME_INITIALIZER { 0 } /*-**************************************** @@ -73,16 +44,23 @@ extern "C" { ******************************************/ UTIL_time_t UTIL_getTime(void); + +/* Timer resolution can be low on some platforms. + * To improve accuracy, it's recommended to wait for a new tick + * before starting benchmark measurements */ void UTIL_waitForNextTick(void); +/* tells if timefn will return correct time measurements + * in presence of multi-threaded workload. + * note : this is not the case if only C90 clock_t measurements are available */ +int UTIL_support_MT_measurements(void); PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd); PTime UTIL_clockSpanNano(UTIL_time_t clockStart); -#define SEC_TO_MICRO ((PTime)1000000) PTime UTIL_getSpanTimeMicro(UTIL_time_t clockStart, UTIL_time_t clockEnd); PTime UTIL_clockSpanMicro(UTIL_time_t clockStart); - +#define SEC_TO_MICRO ((PTime)1000000) /* nb of microseconds in a second */ #if defined (__cplusplus) diff --git a/tests/decodecorpus.c b/tests/decodecorpus.c index 15e89d51f..86bbaf8bf 100644 --- a/tests/decodecorpus.c +++ b/tests/decodecorpus.c @@ -25,21 +25,13 @@ #include "zdict.h" /* Direct access to internal compression functions is required */ -#include "zstd_compress.c" /* ZSTD_resetSeqStore, ZSTD_storeSeq, *_TO_OFFBASE, HIST_countFast_wksp, HIST_isError */ +#include "compress/zstd_compress.c" /* ZSTD_resetSeqStore, ZSTD_storeSeq, *_TO_OFFBASE, HIST_countFast_wksp, HIST_isError */ #define XXH_STATIC_LINKING_ONLY #include "xxhash.h" /* XXH64 */ -#ifndef MIN - #define MIN(a, b) ((a) < (b) ? (a) : (b)) -#endif - -#ifndef MAX_PATH - #ifdef PATH_MAX - #define MAX_PATH PATH_MAX - #else - #define MAX_PATH 256 - #endif +#if !(defined (__cplusplus) || (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) /* C99 */)) +# define inline /* disable */ #endif /*-************************************ @@ -71,6 +63,7 @@ static UTIL_time_t g_displayClock = UTIL_TIME_INITIALIZER; } \ } while (0) + /*-******************************************************* * Random function *********************************************************/ @@ -176,6 +169,14 @@ const char* BLOCK_TYPES[] = {"raw", "rle", "compressed"}; #define MIN_SEQ_LEN (3) #define MAX_NB_SEQ ((ZSTD_BLOCKSIZE_MAX + MIN_SEQ_LEN - 1) / MIN_SEQ_LEN) +#ifndef MAX_PATH + #ifdef PATH_MAX + #define MAX_PATH PATH_MAX + #else + #define MAX_PATH 256 + #endif +#endif + BYTE CONTENT_BUFFER[MAX_DECOMPRESSED_SIZE]; BYTE FRAME_BUFFER[MAX_DECOMPRESSED_SIZE * 2]; BYTE LITERAL_BUFFER[ZSTD_BLOCKSIZE_MAX]; @@ -241,6 +242,10 @@ typedef enum { gt_block, /* generate compressed blocks without block/frame headers */ } genType_e; +#ifndef MIN + #define MIN(a, b) ((a) < (b) ? (a) : (b)) +#endif + /*-******************************************************* * Global variables (set from command line) *********************************************************/ diff --git a/tests/external_matchfinder.h b/tests/external_matchfinder.h index 041f73e4d..e4f7c95f0 100644 --- a/tests/external_matchfinder.h +++ b/tests/external_matchfinder.h @@ -32,4 +32,4 @@ size_t zstreamExternalMatchFinder( size_t windowSize ); -#endif // EXTERNAL_MATCHFINDER +#endif /* EXTERNAL_MATCHFINDER */ diff --git a/tests/fuzzer.c b/tests/fuzzer.c index ce0bea573..cb46dc45b 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -476,7 +476,7 @@ static void test_compressBound(unsigned tnb) CHECK_EQ(ZSTD_compressBound(w), ZSTD_COMPRESSBOUND(w)); } } - // Ensure error if srcSize too big + /* Ensure error if srcSize too big */ { size_t const w = ZSTD_MAX_INPUT_SIZE + 1; CHECK(ZSTD_isError(ZSTD_compressBound(w))); /* must fail */ CHECK_EQ(ZSTD_COMPRESSBOUND(w), 0); @@ -489,7 +489,7 @@ static void test_decompressBound(unsigned tnb) { DISPLAYLEVEL(3, "test%3u : decompressBound : ", tnb); - // Simple compression, with size : should provide size; + /* Simple compression, with size : should provide size; */ { const char example[] = "abcd"; char cBuffer[ZSTD_COMPRESSBOUND(sizeof(example))]; size_t const cSize = ZSTD_compress(cBuffer, sizeof(cBuffer), example, sizeof(example), 0); @@ -497,7 +497,7 @@ static void test_decompressBound(unsigned tnb) CHECK_EQ(ZSTD_decompressBound(cBuffer, cSize), (unsigned long long)sizeof(example)); } - // Simple small compression without size : should provide 1 block size + /* Simple small compression without size : should provide 1 block size */ { char cBuffer[ZSTD_COMPRESSBOUND(0)]; ZSTD_outBuffer out = { cBuffer, sizeof(cBuffer), 0 }; ZSTD_inBuffer in = { NULL, 0, 0 }; @@ -510,14 +510,14 @@ static void test_decompressBound(unsigned tnb) ZSTD_freeCCtx(cctx); } - // Attempt to overflow 32-bit intermediate multiplication result - // This requires dBound >= 4 GB, aka 2^32. - // This requires 2^32 / 2^17 = 2^15 blocks - // => create 2^15 blocks (can be empty, or just 1 byte). + /* Attempt to overflow 32-bit intermediate multiplication result + * This requires dBound >= 4 GB, aka 2^32. + * This requires 2^32 / 2^17 = 2^15 blocks + * => create 2^15 blocks (can be empty, or just 1 byte). */ { const char input[] = "a"; size_t const nbBlocks = (1 << 15) + 1; size_t blockNb; - size_t const outCapacity = 1 << 18; // large margin + size_t const outCapacity = 1 << 18; /* large margin */ char* const outBuffer = malloc (outCapacity); ZSTD_outBuffer out = { outBuffer, outCapacity, 0 }; ZSTD_CCtx* const cctx = ZSTD_createCCtx(); @@ -3535,7 +3535,7 @@ static int basicUnitTests(U32 const seed, double compressibility) DISPLAYLEVEL(3, "test%3i : testing bitwise intrinsics PR#3045: ", testNb++); { - U32 seed_copy = seed; // need non-const seed to avoid compiler warning for FUZ_rand(&seed) + U32 seed_copy = seed; /* need non-const seed to avoid compiler warning for FUZ_rand(&seed) */ U32 rand32 = FUZ_rand(&seed_copy); U64 rand64 = ((U64)FUZ_rand(&seed_copy) << 32) | FUZ_rand(&seed_copy); U32 lowbit_only_32 = 1; @@ -3543,8 +3543,8 @@ static int basicUnitTests(U32 const seed, double compressibility) U32 highbit_only_32 = (U32)1 << 31; U64 highbit_only_64 = (U64)1 << 63; U32 i; - if (rand32 == 0) rand32 = 1; // CLZ and CTZ are undefined on 0 - if (rand64 == 0) rand64 = 1; // CLZ and CTZ are undefined on 0 + if (rand32 == 0) rand32 = 1; /* CLZ and CTZ are undefined on 0 */ + if (rand64 == 0) rand64 = 1; /* CLZ and CTZ are undefined on 0 */ /* Test ZSTD_countTrailingZeros32 */ CHECK_EQ(ZSTD_countTrailingZeros32(lowbit_only_32), 0u);