fix #4332: setting ZSTD_NBTHREADS=0 via environment variable #4334
Conversation
| @@ -10,7 +10,3 @@ zstd -T0 -f file -q ; zstd -t file.zst | |||
| zstd -T0 --auto-threads=logical -f file -q ; zstd -t file.zst | |||
| zstd -T0 --auto-threads=physical -f file -q ; zstd -t file.zst | |||
| zstd -T0 --jobsize=1M -f file -q ; zstd -t file.zst | |||
There was a problem hiding this comment.
Should we add a test here that tests the environment variable works?
If we print the number of threads under some verbosity we could do something like:
ZSTD_NBTHREADS=0 zstd -f file -vv 2>&1 | grep "physical core(s) detected"
zstd -tq file.zst
ZSTD_NBTHREADS=4 zstd -f file -vv 2>&1 | grep "4 threads"
zstd -tq file.zst
There was a problem hiding this comment.
Yes, definitely,
I was considering to do it in another PR, in order to observe the test failure under current conditions, i.e. without the patch.
But I could also just add it to this PR as a follow up commit.
| if ((operation==zom_decompress) && (nbWorkers > 1)) { | ||
| DISPLAYLEVEL(2, "Warning : decompression does not support multi-threading\n"); | ||
| } |
There was a problem hiding this comment.
This was added due to Issue #2918. This may be a useful warning to keep.
There was a problem hiding this comment.
Issue is, with current code,
there is nothing to tell that nbWorkers wasn't set.
Whether it's set via the environment variable, or via the core count detection, it's necessarily set to "some value".
There is no difference with a user setting this value manually via command line.
Therefore, on any modern system with >= 8 logical cores, nbWorkers >= 2, and consequently always triggers this warning.
This could be solved by adding yet another variable, which would track if nbWorkers was set explicitly, or if it happens to be set to a value >1 just as a consequence of automatic core count detection. We would also have to decide if setting nbWorkers via environment variable count or not to trigger this warning.
So it's a bit more work, a bit more questions, and specific to the topic of displaying a warning during decompression mode. It felt it could deserve its own PR for proper focus.
|
test added in this PR, |
Cyan4973
force-pushed
the
fix4332
branch
15 times, most recently
from
848a904 to
5907c45
Compare
use an alias instead of a function also: added more traces and updated version nb to v1.5.8
|
In the process, fixed a latent limitation of the test shell script |
Since
v1.5.7, settingZSTD_NBTHREADS=0via environment variable doesn't work, due to a logic mistake in the initialization routine.Reported in #4332, by JcKlomp.