[PATCH] D39355: [dsymutil] Implement the --threads option

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: tools/dsymutil/dsymutil.cpp:70
+    init(0), cat(DsymCategory));
+static alias ThreadsA("t", desc("Alias for --threads"), aliasopt(Threads));
+
----------------
Could you check that dsymutil produces the same output for some test input with/without threading enabled?


================
Comment at: tools/dsymutil/dsymutil.cpp:330
+      NumThreads = std::min(llvm::thread::hardware_concurrency(),
+                            (unsigned int)DebugMapPtrsOrErr->size());
+    if (DumpDebugMap || Verbose)
----------------
If NumThreads isn't set, why not set it to hardware_concurrency()? Later, we can set NumThreads to the min of itself and the debug map size, since it seems like there's no need to spawn any more threads than that.


================
Comment at: tools/dsymutil/dsymutil.cpp:356
+        if (OutputFile.empty() || !linkDwarf(OutputFile, *Map, Options))
+          exitDsymutil(1);
+      };
----------------
I think there's a chance that an exit from one thread can leave partially-written files from another thread on the FS. Maybe as a first step, we should migrate to ToolOutputFile?


================
Comment at: tools/dsymutil/dsymutil.cpp:365
+      else
+        Threads.async(LinkLambda);
 
----------------
Wdyt of wrapping the ThreadPool in a helper class that only constructs the pool if NumThreads > 1? It's not a big deal, but it's strange to me that we'd spawn/tear down a thread but never use it.


Repository:
  rL LLVM

https://reviews.llvm.org/D39355