Async write for decompression by yoniko · Pull Request #2975 · facebook/zstd

yoniko · 2022-01-05T22:22:32Z

Changes:

Added --[no-]asyncio flag for CLI decompression.
Replaced dstBuffer in decompression with a pool of write jobs.
Added an ability to execute write jobs in a separate thread.
Added an ability to wait (join) on all jobs in a thread pool (queue and running).
Fixed bug in multiframe decompression in lz4.

Benchmarked total runtime (real time) of decompression on 3 machines, these are the runtime improvements in %:

flag                     --asyncio  --no-asyncio  improvement
input_file      machine
enwik8.zst      desktop      0.168         0.211    20.379147
                macbook      0.296         0.556    46.762590
                server       0.195         0.241    19.087137
silesia.tar.zst desktop      0.285         0.388    26.546392
                macbook      0.634         0.982    35.437882
                server       0.337         0.439    23.234624

Notes:

user & system time / cpu utilization is mostly the same and might even be higher, but total time is reduced because we wait less time on IO
Impact might be even bigger on bigger files as we also measure initialization here

terrelln · 2022-01-05T22:48:17Z

Added --[no-]asyncio flag for CLI decompression.

This should probably support compression too. And be overridden if the number of threads is explicitly set with -T.

yoniko · 2022-01-05T22:57:18Z

That's a good point.
Taking it a bit further, perhaps it'd be better to just consolidate with --single-threaded?
The only problem there would be that the --single-threaded has an opposite default than --asyncio has at the moment (we wanted to have it off by default as its still experimental).

terrelln · 2022-01-05T23:14:02Z

Yeah, I think we should combine the functionality with --single-thread. --no-asyncio and --single-thread should just be aliases.

We can default compression to --asyncio and decompression to --no-asyncio, until we're ready to enable it by default.

Currently, --single-thread only applies when -T# is set to 0 or unset. It also doesn't apply if ZSTD_NBTHREADS is not zero. I don't think we actually want ZSTD_NBTHREADS to override --single-thread, but that is open for discussion.

Point being, we should think carefully about the interactions between --single-thread, --[no-]asyncio, -T#, and ZSTD_NBTHREADS. And make sure that we have a consistent story that we're happy with. And that it can be extended to multi-threaded decompression, if we ever add that.

Cyan4973 · 2022-01-05T23:45:18Z

Currently, --single-thread only applies when -T# is set to 0 or unset. It also doesn't apply if ZSTD_NBTHREADS is not zero.

ZSTD_NBTHREADS is only meant to be a "default" value for -T#, therefore it should be overwritten by any explicit command, including -T# or --single-thread.

Currently, --single-thread only applies when -T# is set to 0 or unset.

This seems strange to me, likely an overlook.
I believe it should supersede -T#,
and there is probably a point to output a warning message in case --single-thread is combined with -T2.

Another candidate policy could be "last one wins", meaning that --single-thread -T2 actually means -T2, and -T2 --single-thread actually means --single-thread. Here also, a warning message would be welcomed to underline the discrepancy.

Lastly, a last policy could be to refuse to work in presence of a discrepancy.

I tend to prefer the first policy (--single-thread has priority), but this is certainly opened to discussion.

terrelln

Overall this is looking great!

Could you also add some benchmark figures to the PR summary?

lib/common/pool.c

terrelln · 2022-01-05T23:18:27Z

programs/fileio.c

+    POOL_ctx* writerPool;
+    ZSTD_pthread_mutex_t writeJobsMutex;
+    void* jobs[DECOMPRESSION_MAX_WRITE_JOBS];
+    volatile int availableWriteJobs;


availableWriteJobs does not need to be volatile.

But can you add a newline before writeJobsMutex and after availableWriteJobs, and a comment explaining that the mutex protects jobs and availableWriteJobs?

terrelln · 2022-01-05T23:24:00Z

programs/fileio.c


+static void FIO_writePoolSetDstFile(write_pool_ctx_t *ctx, FILE* dstFile) {
+    assert(ctx!=NULL);
+    // We can change the dst file only if we have finished writing


nit: These need to be /* */ comments. // comments are C99.

terrelln · 2022-01-05T23:41:35Z

programs/fileio.c

+    return job;
+}
+
+static void FIO_writePoolCreateThreadPool(write_pool_ctx_t *ctx, const FIO_prefs_t *prefs) {


nit: naming: Can you name the functions FIO_WritePool_createThreadPool()? Or just WritePool_createThreadPool() since they're all static. That makes it clear its the function createThreadPool() for the object/concept/group WritePool.

terrelln · 2022-01-05T23:42:00Z

programs/fileio.c

+    }
+}
+
+static write_pool_ctx_t* FIO_writePoolCreate(FIO_prefs_t* const prefs) {


WritePool_create()

terrelln · 2022-01-05T23:42:17Z

programs/fileio.c

    }   }
 }

+static void FIO_writePoolSetDstFile(write_pool_ctx_t *ctx, FILE* dstFile) {


WritePool_setDstFile(), etc.

terrelln · 2022-01-05T23:45:31Z

programs/fileio.c

+        FIO_WritePoolWriteJobExecute(job);
+}
+
+static void FIO_writePoolQueueWriteJobAndGetNextAvailable(write_job_t **job) {


For all the functions you intend to be "public", e.g. not the helper functions only used by the other WritePool functions, could you add documentation? You could also add docs for the helpers, but that is less necessary.

terrelln · 2022-01-05T23:50:09Z

I tend to prefer the first policy (--single-thread has priority), but this is certainly opened to discussion.

I agree that --single-thread should certainly have priority over ZSTD_NBTHREADS. I could see it going either way for -T#, and don't have a strong preference, as long as we are consistent.

If we decide that --single-thread should override -T#, should --no-asyncio also override -T#?

Cyan4973 · 2022-01-06T00:40:05Z

If we decide that --single-thread should override -T#, should --no-asyncio also override -T#?

If we decide that --single-thread == --no-asyncio, then yes.

There is a catch though.
--single-thread produces a compressed output different from -T1.
if --no-asyncio == --single-thread, then it will have the same impact,
which has consequences from a reproducibility perspective.
Yet, while it's reasonable to explain that --single-thread impacts the compression process, hence impacts the compressed output, it's more difficult to use the same argument for --no-asyncio, which should only impact I/O.

Thoughts ?

Cyan4973 · 2022-01-06T01:45:56Z

Could you add a test featuring multi-frames files ?
It's currently a supported scenario, and it's possibly one of the difficult scenarios for --async-io.

Bonus point for files with multiple heterogeneous frames (also currently supported).

yoniko · 2022-01-06T01:48:38Z

I've actually tested this manually, but it makes sense to add an automated test. Will do it.

yoniko · 2022-01-06T18:56:24Z

Updating here that we've spoken about --single-thread and --[no-]asyncio and decided to leave the two uncoupled as they have two different underlining meanings:

-T# and --single-thread control the threading mode for compression which directly affects the result of compression itself. Thus the --single-thread flag's importance is in compressing in a single-threaded mode and not about having the cli actually run in a single thread.
--[no-]asyncio is about having asyncio interfaces for (de)compression. These asyncio interfaces could work with a single threaded compression as well (if available).

Cyan4973 · 2022-01-06T20:03:01Z

tests/playTests.sh

+  testAsyncIO lz4
+  addTwoFrames lz4
+fi
+cat tmp_uncompressed | shasum > tmp2


We use $MD5SUM to designate the checksum program,
which generally translates into md5sum, but not always (freeBSD uses gmd5sum, macos uses md5 -r, etc.).

Thank you, pushed a fix.

Cyan4973 · 2022-01-06T21:46:24Z

I like the new code path.
I believe it fits well in current design, and even clarifies the processing pattern.
Evidence is that it can be generalized with minimal effort to all supported decoders.

The last issue is about the meson build script,
which I suspect is related to meson's contributors insistence of building the CLI against the shared library,
even though it is a scenario we explicitly do not support.
This PR makes the situation a bit worse in this regard, since it makes the CLI depend even more on non-public components of libzstd, which is fine as long as CLI is linked to the static library, but not the dynamic one.

It may seem a unrelated secondary topic, but we'll nonetheless need a solution to this situation before the merge.

yoniko · 2022-01-11T02:43:03Z

meson build fixed and Travis finishes successfully.
I did need to include some more files from lib/common in the building of the binary.

- Added --[no-]asyncio flag for CLI decompression. - Replaced dstBuffer in decompression with a pool of write jobs. - Added an ability to execute write jobs in a separate thread. - Added an ability to wait (join) on all jobs in a thread pool (queue and running).

facebook-github-bot added the CLA Signed label Jan 5, 2022

terrelln reviewed Jan 5, 2022

View reviewed changes

Cyan4973 reviewed Jan 6, 2022

View reviewed changes

yoniko force-pushed the decompression_asyncio_pool branch from 7d64b55 to 2ce5c4c Compare January 6, 2022 20:10

yoniko force-pushed the decompression_asyncio_pool branch from c3817cc to bfc188d Compare January 11, 2022 02:23

yoniko added 6 commits January 21, 2022 12:02

Async IO decompression:

5742475

- Added --[no-]asyncio flag for CLI decompression. - Replaced dstBuffer in decompression with a pool of write jobs. - Added an ability to execute write jobs in a separate thread. - Added an ability to wait (join) on all jobs in a thread pool (queue and running).

Async IO decompression: CR fixes

86ec45c

LZ4 decompression: Fixed multiframe decompression

ac90062

Async IO decompression: Added multiframe test for --[no-]asyncio

873cb56

Async IO decompression: C standard fix

f0e01a5

Async IO decompression: fix meson compilation

30152d4

yoniko force-pushed the decompression_asyncio_pool branch from bfc188d to 30152d4 Compare January 21, 2022 20:02

yoniko merged commit 1598e6c into facebook:dev Jan 21, 2022

Cyan4973 mentioned this pull request Feb 9, 2023

release v1.5.4 #3487

Merged

Conversation

yoniko commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terrelln commented Jan 5, 2022

Uh oh!

yoniko commented Jan 5, 2022

Uh oh!

terrelln commented Jan 5, 2022

Uh oh!

Cyan4973 commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terrelln left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrelln commented Jan 5, 2022

Uh oh!

Cyan4973 commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cyan4973 commented Jan 6, 2022

Uh oh!

yoniko commented Jan 6, 2022

Uh oh!

yoniko commented Jan 6, 2022

Uh oh!

Cyan4973 Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cyan4973 commented Jan 6, 2022

Uh oh!

yoniko commented Jan 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yoniko commented Jan 5, 2022 •

edited

Loading

Cyan4973 commented Jan 5, 2022 •

edited

Loading

Cyan4973 commented Jan 6, 2022 •

edited

Loading

Cyan4973 Jan 6, 2022 •

edited

Loading