Skip to content

Change CLI to employ multithreading by default - #4211

Merged
daniellerozenblit merged 2 commits into
meta:devfrom
daniellerozenblit:mt-cli
Dec 12, 2024
Merged

daniellerozenblit merged 2 commits into
meta:devfrom
daniellerozenblit:mt-cli

Conversation

@daniellerozenblit

@daniellerozenblit daniellerozenblit commented Dec 10, 2024 •

Copy link
Copy Markdown
Contributor

Summary

This PR modifies ZSTDCLI_NBTHREADS_DEFAULT so that multithreading is used by default when the library is compiled with multithreading support.

We use a conservative value of max(1, min(4, nbCores/4)) worker threads for this default value.

Testing

I benchmarked on an Intel(R) Xeon(R) Gold 6138 CPU @ 2.00GHz machine with 80 logical cores. I observed that the default behavior is now similar to the behavior when explicitly doing multi-threaded compression using the -T flag.

~/zstd$ ./zstd -b3 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66216521 (x3.201),  358.2 MB/s,  891.2 MB/s
~/zstd$ ./zstd -b3 -T4 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66216521 (x3.201),  323.2 MB/s,  885.1 MB/s
~/zstd$ ./zstd -b3 -T2 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66216521 (x3.201),  266.1 MB/s,  885.0 MB/s
~/zstd$ ./zstd -b3 -T1 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66137723 (x3.205),  189.6 MB/s,  863.7 MB/s

Comment thread programs/zstdcli.c Resolved
Comment thread programs/zstd.1.md Resolved
@Cyan4973

Cyan4973 commented Dec 10, 2024 •

Copy link
Copy Markdown
Contributor
~/zstd$ ./zstd -b3 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66216521 (x3.201),  358.2 MB/s,  891.2 MB/s
~/zstd$ ./zstd -b3 -T4 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66216521 (x3.201),  323.2 MB/s,  885.1 MB/s

The difference between these 2 measurements is > 10%, which is quite substantial if they are expected to be equal.
Is it repeatable, or is it a one-off ?

Also:
I would expect -vv to specify clearly the nb of threads used.

@Cyan4973

Copy link
Copy Markdown
Contributor

Also as a follow-up,
it would be good to clarify the (potential) impact on the benchmark mode (-b).

@daniellerozenblit

Copy link
Copy Markdown
Contributor Author
~/zstd$ ./zstd -b3 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66216521 (x3.201),  358.2 MB/s,  891.2 MB/s
~/zstd$ ./zstd -b3 -T4 ~/silesia/silesia/*
 3# 12 files         : 211938580 ->  66216521 (x3.201),  323.2 MB/s,  885.1 MB/s

The difference between these 2 measurements is > 10%, which is quite substantial if they are expected to be equal. Is it repeatable, or is it a one-off ?

Also: I would expect -vv to specify clearly the nb of threads used.

I re-did this experiment, compressing silesia with the default number threads (which on my machine evaluates to 4) and then explicitly passing -T4 and got more consistent results.

Overall it seems that there is more noise in the benchmarks with multithreading, but the behavior looks overall as expected.

# -T4
 3# 12 files         : 211938580 ->  66216521 (x3.201),  310.7 MB/s,  861.7 MB/s
 3# 12 files         : 211938580 ->  66216521 (x3.201),  306.1 MB/s,  862.6 MB/s
 3# 12 files         : 211938580 ->  66216521 (x3.201),  318.3 MB/s,  868.3 MB/s

# default
 3# 12 files         : 211938580 ->  66216521 (x3.201),  316.1 MB/s,  871.2 MB/s
 3# 12 files         : 211938580 ->  66216521 (x3.201),  315.3 MB/s,  867.9 MB/s
 3# 12 files         : 211938580 ->  66216521 (x3.201),  307.5 MB/s,  866.8 MB/s

Comment thread programs/zstdcli.c Outdated
}
}
DISPLAYLEVEL(2, "Compressing with %u worker threads \n", nbWorkers);
DISPLAYLEVEL(3, "Compressing with %u worker threads \n", nbWorkers);

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.

Yes, level 3 seems better

@daniellerozenblit
daniellerozenblit force-pushed the mt-cli branch 2 times, most recently from 611e4f6 to 2a8502e Compare December 11, 2024 22:04
@daniellerozenblit
daniellerozenblit marked this pull request as draft December 11, 2024 22:25
… level >= 4, and add lower bound of 1 for the default number of threads
@daniellerozenblit
daniellerozenblit marked this pull request as ready for review December 12, 2024 16:40
@daniellerozenblit
daniellerozenblit merged commit 17beeb5 into meta:dev Dec 12, 2024
ernetas added a commit to ernetas/compression-bench that referenced this pull request Sep 11, 2026
I asserted that DataDog/zstd exposes no way to set ZSTD_c_nbWorkers. That
is false: zstd_stream.go:291 has SetNbWorkers. The bench simply never
called it, so the zstd-cgo row was a one-worker baseline by omission, not
by constraint. zstd-cgo-mt is the same binding with it set, which makes
libzstd's multithreading reachable in-process instead of only through the
CLI. On a 274MiB tar: 389 MB/s at 1.02x cores for zstd-cgo against
2380 MB/s at 7.47x cores for zstd-cgo-mt.

Note the worker count cannot be 0. At the library level
ZSTD_c_nbWorkers=0 means *no* worker threads; the core detection behind
the CLI's -T0 lives in programs/zstdcli.c, not in libzstd. Passing 0
silently yields a single-threaded encoder, measured at 1.03x cores and
indistinguishable from the plain binding. It is set to GOMAXPROCS here,
matching what pgzip and klauspost's zstd default to. The MT variant is
registered only when the linked libzstd actually has threading, so a
library built without it records a skip rather than failing the run.

Also: the CLI became multi-threaded by default in v1.5.7
(meta/zstd#4211), capped at max(1, min(4, nbCores/4)), not "since
1.5.x". That bound matters -- on <= 1.5.6 a bare `zstd -{level}` really
was single-threaded, so a CI running an older CLI would not show the
problem this branch is correcting for.
jlee303 pushed a commit that referenced this pull request Nov 13, 2026
…mode

- -T# default is ZSTDCLI_NBTHREADS_DEFAULT, based on logical cores
  (UTIL_countLogicalCores() / 4, since #4211), not physical cores.
- ZSTDMT_NBWORKERS_MAX is 64 on 32-bit and 256 on 64-bit, not 200.
- Benchmark mode uses 1 worker thread by default since 725a152.

Signed-off-by: YoungjuKim <[email protected]>
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