zstd Support OpenMP

divsufsort can use OpenMP. Is there a reason not to use this option in dictBuilder?

Mar 17 '21 09:03 data-man

Or migrate to https://github.com/IlyaGrebnov/libsais.

Mar 17 '21 09:03 data-man

We try to reduce dependencies to a minimum within the library. openmp is a giant one.

For this module, migration to a newer module is an acceptable option, especially when it improves performance.

That being said, another rule of libzstd code base is to remain C90 compatible (+ long long support). C99 doesn't pass that. And unfortunately, libsais advertises being C99. So it would typically require a translation, which might make the resulting code out of sync with upstream for future maintenance. Hence a sustained non-trivial effort.

Finally, the bwt transform is only used in slow mode in the dictionary builder. Faster modes use an entirely different algorithm.

All this to say: I'm not against such a change, but it's significant work, and not high-pri.

Mar 17 '21 16:03 Cyan4973

Thanks for detailed (as always) answer!

openmp is a giant one.

It can be useful with embedded zstd (e.g. with single_file_libs scripts). Thus, the user can decide for himself whether to use OpenMP.

Mar 17 '21 16:03 data-man

Some thoughts:

add ZSTD_ENABLE_OPENMP:BOOL=OFF (or ZSTD_USE_OPENMP:BOOL=OFF) to CMake config
add the same for makefile

Mar 19 '21 16:03 data-man

I would be okay accepting a PR that enabled OpenMP support for divsufsort, as long as it is hidden behind a define that defaults to disabled. We can then thread that define through make/cmake as you suggest.

Mar 19 '21 17:03 terrelln

Or migrate to IlyaGrebnov/libsais.

My first attempt: https://github.com/data-man/zstd/tree/libsais :smile: Please try with $ make USE_OPENMP=1.

Dec 19 '21 15:12 data-man