[PATCH] D131247: [ELF] Parallelize writes of different OutputSections

[Andrew Ng via Phabricator via llvm-commits]

andrewng added a comment.

I assume that the performance figures are based on running on Linux. Do you have any idea what the performance impact is on Windows?



================
Comment at: lld/ELF/OutputSections.cpp:461
+      if (!std::exchange(written, true))
+        parallelFor(0, sections.size(), fn);
       writeInt(buf + data->offset, data->expression().getValue(), data->size);
----------------
Given there is already a "live" `TaskGroup`, I don't think this will actually run in parallel, IIUC. However, this is a limitation of the current parallel implementation.


================
Comment at: lld/ELF/OutputSections.cpp:471
+  // non-deterministic.
+  asyncParallelFor(tg, 128, 0, sections.size(), fn);
 }
----------------
MaskRay wrote:
> ikudrin wrote:
> > Where does the value of `128` come from? It looks like, with the hardcoded value, large projects will benefit from the parallelization more than small ones. And it also does not take into account the `llvm::parallel::detail::MaxTasksPerGroup`; can it somehow be derived from the common setting?
> I don't think it can be derived from the common setting. It may not be bad to have a call site specific value like `SmallVector`. If we derived this automatically, tuning the library parameter may be more difficult.
I would actually suggest that the task size could be even larger than `128` for output sections that have many input sections. I think as a "minimum" setting it feels about right. It's usually hard to derive a good task size value without incurring some cost in gathering appropriate metrics in order to make a good estimate.

Perhaps the task size could be calculated based on the number of sections and the number of threads in the task pool?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131247