Fix zstdmt stability issues and clean up the zstdmt code #2339
Conversation
|
After ~400K iterations I saw some non-determinism. But it isn't reproducible. I have to figure out if it is a problem with the test or zstdmt. |
terrelln
force-pushed
the
zstdmt-stability
branch
from
91c487f to
6a50dae
Compare
|
I believe I've found all the sources of non-determinism. Running 3 x 400K tests now. |
|
There is some non-determinism left. It is pretty rare. I would be okay merging the test as-is, though it may occasionally fail. I'm running the tests with tracing enabled now. Hopefully it will eventually fail and I can get a trace of what is happening to narrow it down and fix it. |
| * This is a blocking function : it will only give back control to caller after finishing its compression job. | ||
| */ | ||
| static size_t | ||
| ZSTDMT_compress_advanced_internal( |
There was a problem hiding this comment.
Early in the life of zstdmt, one of the benefits of this function
is that it was able to compress directly from src into dst
provided there is enough dstCapacity (<= ZSTD_compressBound(srcSize)),
thus skipping the need to use intermediate buffers,
thus saving memory, and some unnecessary memcpy().
With this function removed,
is the replacement able to retain this property ?
There was a problem hiding this comment.
is the replacement able to retain this property ?
No. We already weren't saving memory because when using the streaming API we have already allocated all of our buffers at this point. With this change, we are doing one extra memcpy for the source buffers.
lib/compress/zstdmt_compress.c
Outdated
| if ((input->pos < input->size) && (endOp == ZSTD_e_end)) { | ||
| /* Can't end yet because the input is not fully consumed. | ||
| * We are in one of these cases: | ||
| * - Input buffer is NULL & empty |
There was a problem hiding this comment.
note : if input buffer is NULL,
we should also have :
input->pos == 0 == input->size,
therefore, this scenario shouldn't pass the if() test (strict < size comparison).
There was a problem hiding this comment.
I should clarify the comment. I mean the mtctx->inBuff.
|
This is a pretty good simplification. I mostly have a question regarding the memory usage of the replacement function for one-shot compression. |
The memory usage isn't impacted, since there isn't a way to avoid allocating all the buffers using the zstd API. I think there is opportunity for I think that should be a follow up PR though, since this doesn't regress the current state of the world (except for 1 more memcpy() of the source). |
|
I've opened issue #2353 to handle the memory saving optimizations. |
This is already handled by zstd, so this logic is never used.
Simplifies the code and removes blocking from zstdmt. At this point we could completely delete `ZSTDMT_compress_advanced_internal()`. However I'm leaving it in because I think we want to do that in the zstd-1.5.0 release, in case anyone is still using the ZSTDMT API, even though it is not installed by default. Fixes facebook#2327.
terrelln
force-pushed
the
zstdmt-stability
branch
from
6a50dae to
441ce41
Compare
This commit leaves only the functions used by zstd_compress.c. All other functions have been removed from the API. The ZSTDMT unit tests in fuzzer.c and zstreamtest.c have been rewritten to use the ZSTD API. And the --mt zstreamtest tests have been ripped out.
* Run compression twice and check the compressed data is byte-identical. The compression loop had to be rewritten to ensure deteriminism. It is guaranteed by always making maximal forward progress. * When nbWorkers > 0, change the number of workers 1/8 of the time. * Run in single-pass mode 1/4 of the time. I've run a few hundred thousand iterations of zstreamtest and have seen no deteriminism issues so far. Before the zstdmt fix that skips the single-pass shortcut non-determinism showed up in a few hundred iterations.
When zstdmt cannot get a buffer and `ZSTD_e_end` is passed an empty compression job can be created. Additionally, `mtctx->frameEnded` can be set to 1, which could potentially cause problems like unterminated blocks. The fix is to adjust to `ZSTD_e_flush` even when we can't get a buffer.
The problem occurs in this scenario: 1. We find a synchronization point. 2. We attmept to create the job. 3. We fail because the job table is full: `mtctx->nextJobID > mtctx->doneJobID + mtctx->jobIDMask`. 4. We call `ZSTDMT_compressStream_generic` again. 5. We forget that we're at a sync point already, and we continue looking for the next sync point. This fix is to detect if we're currently paused at a sync point, and if we are then don't load any more input. Caught by zstreamtest. I modified it to make the bug occur more often (~1/100K -> ~1/200) and verified that it is fixed after. I then ran a few hundred thousand unmodified zstreamtest iterations to verify.
facebook#2339 removes the single-pass zstdmt API. This changes the compressed size, because we no longer take the # of threads into account when deciding the job size.
ghost
mentioned this pull request
singleBlockingThreadmode because it is already handled inzstd_compress.c.zstreamtest --mttest. Rewrite unit tests to use the zstd API.zstreamtestfuzzer loop to check for compression determinism. It had to be rewritten to always make maximal forward progress to ensure determinism in multithreading mode.Bug fixes (the last 2 commits: see commit messages):
ZSTD_e_endand when it can't get an input buffer. It would also incorrectly setmtctx->frameEnded = 1, which could cause other issues, like an unterminated frame. The fix is to move the switch toZSTD_e_flushoutside of theif()block.