[PATCH] D133679: [ELF] Parallelize --compress-debug-sections=zstd

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/OutputSections.cpp:344
+#if LLVM_ENABLE_ZSTD
+  if (config->compressDebugSections == DebugCompressionType::Zstd) {
+    // Allocate a buffer of half of the input size, and grow it by 1.5x if
----------------
peter.smith wrote:
> I think this is ZSTD's streaming compression? If I'm right could be worth saying something like:
> // Use ZSTD's streaming compression API which permits parallel workers working on the stream.
> // See http://facebook.github.io/zstd/zstd_manual.html Streaming compression -HowTo.
Thanks for the comment. Adopted.


================
Comment at: lld/ELF/OutputSections.cpp:360
+    do {
+      const size_t n = std::min(size - pos, (size_t)1 << 20);
+      if (n == size - pos)
----------------
peter.smith wrote:
> Could be worth making a const variable for 1 << 20 with a self descriptive name. Otherwise could be worth a comment on the choice of value.
Switched to `ZSTD_CStreamInSize()` to avoid using a magic number. It's a soft recommendation (https://github.com/llvm/llvm-project/issues/57685#issuecomment-1244950193), though.


================
Comment at: lld/ELF/OutputSections.cpp:372
+        more = ZSTD_compressStream2(cctx, &zob, &zib, directive);
+        assert(!ZSTD_isError(more));
+      }
----------------
peter.smith wrote:
> Does ZSTD guarantee no error for the inputs that we are giving it? If we can't guarantee it then perhaps this should be a fatal error message.
>From the source code it's guaranteed if the usage is correct. The author uses an assert, too: https://github.com/llvm/llvm-project/issues/57685#issuecomment-1244008295


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133679/new/

https://reviews.llvm.org/D133679