Skip to content

benchfn's reduced dependencies#1581

Merged
Cyan4973 merged 25 commits into
devfrom
benchfn
Apr 11, 2019
Merged

benchfn's reduced dependencies#1581
Cyan4973 merged 25 commits into
devfrom
benchfn

Conversation

@Cyan4973

@Cyan4973 Cyan4973 commented Apr 10, 2019

Copy link
Copy Markdown
Contributor

benchfn used to rely on mem.h, and util,
which in turn relied on platform.h.
Using benchfn outside of zstd required to bring all these dependencies.

Now, dependency is reduced to timefn only.
This required to create a separate timefn from util,
and rewrite benchfn and timefn to no longer need mem.h nor platform.h.

Separating timefn from util has a wide effect accross the code base,
as usage of time functions is widespread.
A lot of build scripts had to be updated to also include timefn.

to use new benchfn
returning a double.
added a static assert to ensure condition is respected on target platform
and removed some // comment style
benchfn used to rely on mem.h, and util,
which in turn relied on platform.h.
Using benchfn outside of zstd required to bring all these dependencies.

Now, dependency is reduced to timefn only.
This required to create a separate timefn from util,
and rewrite benchfn and timefn to no longer need mem.h.

Separating timefn from util has a wide effect accross the code base,
as usage of time functions is widespread.
A lot of build scripts had to be updated to also include timefn.
using stdlib's perror()
cmake build script: added timefn
C11 mandates the definition of timespec_get() and TIME_UTC.
However, FreeBSD11 announce C11 compliance, but does not provifr timespec_get(),
breaking link stage for benchfn.
Since it does not provide TIME_UTC either, which is also required by C11,
test this macro: this will automatically rule out FreeBSD 11 for this code path
(it will use the backup C90 path instead, based on clock_t).

The issue seeems fixed in FreeBSD 12.
to circumvent Visual's C4804 warning
unused variable when assert() turned off in fileio.c
added poolTests to all
must use ZSTD_ prefix in front of pthread types
so that they get properly translated for Windows.
not sure why, but msan fires an "unitialized variable" error
when time gets properly initialized by timespec_get().
Maybe in some cases, not all bytes of the structure are initialized ?
Or maybe msan fails to detect the initialization ?

Anyway, pre-initializing the variable before passing it to timespec_get() works.
@Cyan4973 Cyan4973 merged commit 8ac2831 into dev Apr 11, 2019
Comment thread programs/timefn.h
typedef PTime UTIL_time_t;
#define UTIL_TIME_INITIALIZER 0

#elif (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) /* C11 */) \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Cyan4973 this is not the right feature test macro. This disables precise timing on all Linux machines not compiling with C11.

It used to be:

(PLATFORM_POSIX_VERSION >= 200112L) \	
   && (defined(__UCLIBC__)                \	
      || (defined(__GLIBC__)              \	
          && ((__GLIBC__ == 2 && __GLIBC_MINOR__ >= 17) \	
             || (__GLIBC__ > 2))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, this was detected by poolTest failing, so I guess that we're not currently running it in any CI.

@Cyan4973 Cyan4973 May 2, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right,
PLATFORM_POSIX_VERSION was a fairly complex macro, set within platform.h,
the result of mixing several other feature test macros and setting some build macros
in some surgical order, with plenty of exceptions.

In contrast, the C11 test, while different, has the benefit of simplicity, and (at least in theory) a larger scope than posix.

The idea is to rely on C90's clock_t when this condition could not be met.

Note that, whatever the code path, the number and nature of symbols defined remains constant. Same functions, same types, same macros.
UTIL_clockSpanNano() and UTIL_getSpanTimeNano() are still supposed to be supported, even with C90's clock_t.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The downside is that clock_t has very low resolution on Linux. Since this function is used in benchmarking, we'll end up with biased results.

@Cyan4973 Cyan4973 May 2, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes.
This felt acceptable.
The design of the benchmark function should reduce the noise to ~1% on "low" resolution timers (10 ms).

A more problematic issue is that multi-thread benchmarks do not work with clock_t : it sums the time spent on each core. That's a more concerning limitation.

To be complete, I'm not against having a "posix branch", the same way there is already a Windows and a Mac OS-X branch.
But the way "posix compatibility" was tested looked a bit too scary for me, and I was afraid of its side-effects and maintenance.

All modern posix systems should be C11 compatible by now, hence will correctly use the new C11 path with high accuracy timers.

So now it's a matter of improving the benchmark accuracy of old posix systems. It did not felt a high priority, but it can certainly be done if considered important enough. At least, with the C11 path now available, the "posix" path only role is to improve timer accuracy for some targeted older systems. This might help to reduce the complexity of the macro test, which was staggering, as it used to be the only way to provide high-accuracy timers to linux and co, and was meant to pass compilation on all sort of unixes, many of which being "partially" compliant in various ways.

@terrelln

terrelln commented May 2, 2019 via email

Copy link
Copy Markdown
Contributor

@terrelln

terrelln commented May 2, 2019

Copy link
Copy Markdown
Contributor

Tried replying by email... it didn’t work out so well.

@facebook facebook deleted a comment from xuhuleon May 2, 2019
@Cyan4973 Cyan4973 deleted the benchfn branch August 27, 2019 17:25
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