[PATCH] D117853: [ELF] Parallelize --compress-debug-sections=zlib

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

No objections from me. I think the speed up is worth the small amount of extra size. I've made a few small suggestions but are all subjective. I don't have a lot of large programs hanging around to test this on. I guess something like Chromium would give you another data point.

If you've not done it yet, would be good to try and open the test program in a debugger to check to see if it decompress the output. I'd expect there to be no problems but could be worth a sanity check.



================
Comment at: lld/ELF/OutputSections.cpp:302
+
+  // Allowcate a buffer of half of the input size, and grow it by 1.5x if
+  // insufficient.
----------------
Typo // Allocate a buffer


================
Comment at: lld/ELF/OutputSections.cpp:358
+  const size_t numShards = (size + shardSize - 1) / shardSize;
+  auto in = std::make_unique<ArrayRef<uint8_t>[]>(numShards);
+  for (size_t idx = 0, i = 0, j; i != buf.size(); ++idx, i = j) {
----------------
Is it worth picking a plural as there can be more than one shard? Similarly for out and adler. For example ins, outs and adlers. I'm not sure ins and outs sound right though, perharps shardsIn and shardsOut. Again not a strong opinion.
 


================
Comment at: lld/ELF/OutputSections.cpp:359
+  auto in = std::make_unique<ArrayRef<uint8_t>[]>(numShards);
+  for (size_t idx = 0, i = 0, j; i != buf.size(); ++idx, i = j) {
+    j = std::min(i + shardSize, buf.size());
----------------
Might be worth using start and end rather than i and j? I've not got a strong opinion here, happy to keep with i, j if you prefer. 


================
Comment at: lld/ELF/OutputSections.cpp:367
+  auto adler = std::make_unique<uint32_t[]>(numShards);
+  parallelForEachN(0, numShards, [&](size_t i) {
+    out[i] = deflateShard(in[i], level, i == numShards - 1);
----------------
The code above use idx for going through in[] and i for something else, could be worth using the same value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117853