[PATCH] D115416: [lld-macho] Prevent writing map files on the critical path

[Vincent Lee via Phabricator via llvm-commits]

thevinster added inline comments.


================
Comment at: lld/MachO/Writer.cpp:1103-1114
+  ThreadPool writeUuidThreadPool;
+
   ArrayRef<uint8_t> data{buffer->getBufferStart(), buffer->getBufferEnd()};
   unsigned chunkCount = parallel::strategy.compute_thread_count() * 10;
   // Round-up integer division
   size_t chunkSize = (data.size() + chunkCount - 1) / chunkCount;
   std::vector<ArrayRef<uint8_t>> chunks = split(data, chunkSize);
----------------
int3 wrote:
> thevinster wrote:
> > oontvoo wrote:
> > > why is this change needed?
> > > 
> > This commit is meant as a way to show the different ways we could be using `ThreadPool`. It's more of an RFC (See https://reviews.llvm.org/D115416#3190992). As far as why we want to change it (per @int3's suggestion), we want to change all our uses of `parallelForEach` in favor of `ThreadPool` so we can control the total number of threads. 
> we should be reusing the `writerThreadPool` here instead of creating a `writeUuidThreadPool`... each ThreadPool maintains its own counter of the number of threads spawned
Okay, so it sounds like we want to go for Option 2 then? 


================
Comment at: lld/MachO/Writer.cpp:68
 
+  ThreadPool writerThreadPool;
   std::unique_ptr<FileOutputBuffer> &buffer;
----------------
int3 wrote:
> thevinster wrote:
> > int3 wrote:
> > > thevinster wrote:
> > > > int3 wrote:
> > > > > super nit, but "writer" here seems redundant since this is the only threadpool used in Writer
> > > > I think it might be more relevant now we that want to use it in `finalizeLinkEditSegment`. Any reason why we want to instantiate it in the `run` scope as opposed to having it as a member of the class? It's possible we may want to use it elsewhere within the `Writer` class? In fact, scanning usages of `parallelForEach{N}` also shows `writeUuid` benefiting from using async as well. In that case, it makes sense to put it at the class level scope. 
> > > I have no issues with the scope. I'm just saying that having "writer" in the variable name seems redundant
> > My bad. I misinterpreted the statement. 👍
> so can we rename `writerThreadPool` to just `threadPool`? :)
I'll clean this up. This revision is more of a "which option do we want to go for first" before I clean up all the nit details


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115416