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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've left some comments based on a first read of the code. The code looks reasonable to me. I'm not in a position to mention if this gives bad peformance on Windows (if there is a ZSTD consumer on that platform anyway) though.



================
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
----------------
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.


================
Comment at: lld/ELF/OutputSections.cpp:360
+    do {
+      const size_t n = std::min(size - pos, (size_t)1 << 20);
+      if (n == size - pos)
----------------
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.


================
Comment at: lld/ELF/OutputSections.cpp:364
+      ZSTD_inBuffer zib = {buf.get() + pos, n, 0};
+      size_t more = 1;
+      while (zib.pos != zib.size || (directive == ZSTD_e_end && more != 0)) {
----------------
more reads like it should be a boolean. Perhaps bytesRemaining?


================
Comment at: lld/ELF/OutputSections.cpp:372
+        more = ZSTD_compressStream2(cctx, &zob, &zib, directive);
+        assert(!ZSTD_isError(more));
+      }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133679