Skip to content

programs/timefn: Don't use clock() on Unix when C < 11 to improve multi-thread benchmark#3165

Merged
Cyan4973 merged 1 commit intofacebook:clock_gettimefrom
jbeich:timespec
Jun 19, 2022
Merged

programs/timefn: Don't use clock() on Unix when C < 11 to improve multi-thread benchmark#3165
Cyan4973 merged 1 commit intofacebook:clock_gettimefrom
jbeich:timespec

Conversation

@jbeich
Copy link

@jbeich jbeich commented Jun 19, 2022 •
edited
Loading

Fixes #3163
See also freebsd/freebsd-src@6882d53b7fb8
See also aosp-mirror/platform_bionic@78e9ebc3b968

C90 aka clock() fallback being suboptimal seems to be known issue:

zstd/programs/timefn.c

Lines 138 to 143 in b7b7edb

#else /* relies on standard C90 (note : clock_t measurements can be wrong when using multi-threading) */
UTIL_time_t UTIL_getTime(void) { return clock(); }
PTime UTIL_getSpanTimeMicro(UTIL_time_t clockStart, UTIL_time_t clockEnd) { return 1000000ULL * (clockEnd - clockStart) / CLOCKS_PER_SEC; }
PTime UTIL_getSpanTimeNano(UTIL_time_t clockStart, UTIL_time_t clockEnd) { return 1000000000ULL * (clockEnd - clockStart) / CLOCKS_PER_SEC; }

@heftig
Copy link

heftig commented Jun 19, 2022 •
edited
Loading

I just came to the same conclusion. The "bad performance" is a measurement error in zstdcli, not an actual performance issue. The libzstd produced with c11 and c99 are identical.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jun 19, 2022 •
edited
Loading

I think this patch goes in the right direction. I'm actually surprised we don't use clock_gettime() already. Maybe it used to be present and was later removed, or maybe I confuse with lz4. Either way, it's a welcome addition, clearly better than clock().

minor :
for a benchmark function, which only cares about relative differences between 2 time measurements, I would have rather expected CLOCK_MONOTONIC.

I understand that CLOCK_REALTIME is closer in meaning to TIME_UTC, but the main reason to use TIME_UTC with timespec_get() is that it's the only base clearly defined in the C11 standard.

edit : I just checked, and indeed clock_gettime() used to be present. Unsure why it was removed later on. Maybe to remove the nb of time measurement variants, possibly.

Cyan4973
Cyan4973 reviewed Jun 19, 2022
View reviewed changes
@Cyan4973
Copy link
Contributor

Cyan4973 commented Jun 19, 2022

Note :
I believe this patch doesn't fix #3163 .

It does fix -std=gnu99 for example, but -std=c99 is too strong and does not define posix-only symbols, such as clock_gettime().

It's not necessarily a problem for this PR, which provides clock_gettime() as a time measurement backup for compliant systems.

It just shows there is a bit more to do to address #3163 specifically.

@jbeich
[programs] Always use timespec on Unices
376b9a0 
- 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
@jbeich
Copy link
Author

jbeich commented Jun 19, 2022

It does fix -std=gnu99 for example, but -std=c99 is too strong and does not define posix-only symbols, such as clock_gettime().

Addressed via file-local _POSIX_C_SOURCE. BSD libc doesn't hide POSIX declarations when using -std=c99, so it's a bit of a blind fix.

Cyan4973
Cyan4973 reviewed Jun 19, 2022
View reviewed changes
programs/timefn.c
* You may select, at your option, one of the above-listed licenses.
*/

#define _POSIX_C_SOURCE 200809L /* clock_gettime */
Copy link
Contributor

@Cyan4973 Cyan4973 Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should rather be defined in timefn.h, just before the #include <time.h>,
otherwise, UTIL_time_t declaration in timefn.h will be out of sync with timefn.c.

edit : although, note the following compilation issue :

../programs/timefn.h:22:0: error: "_POSIX_C_SOURCE" redefined [-Werror]

@Cyan4973
Copy link
Contributor

Cyan4973 commented Jun 19, 2022

With #3163 out of the way, let's now focus on this PR.
It extends the nb of systems that can benefit from a correct timer, so it goes in the right direction.
There are a couple of minor details I would differently,
so instead of nagging you about that,
I'll just merge this PR into a feature branch, and finish off the details there.

@Cyan4973 Cyan4973 changed the base branch from dev to clock_gettime June 19, 2022 23:55
@Cyan4973 Cyan4973 merged commit ae1dea8 into facebook:clock_gettime Jun 19, 2022
Cyan4973 added a commit that referenced this pull request Jun 20, 2022
@Cyan4973
minor modification of #3165
964acd1 
which restore clock_gettime() as a way to get a usable timer for posix systems.
Original PR by @jbeich
Cyan4973 added a commit that referenced this pull request Jun 20, 2022
@Cyan4973
minor modification of #3165
dcab06d 
which restore clock_gettime() as a way to get a usable timer for posix systems.
Original PR by @jbeich
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Cyan4973 Cyan4973 Cyan4973 left review comments

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Strange performance and scalability issues with some of the build systems

4 participants

@jbeich @heftig @Cyan4973 @facebook-github-bot