[PATCH] D53651: [clangd] Use thread pool for background indexing.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Moving forward with priorities then.



================
Comment at: clangd/index/Background.cpp:89
   }
-  QueueCV.notify_all();
+  QueueCV.notify_one();
 }
----------------
sammccall wrote:
> I always forget the details of how these work :-\
> Is it possible for the "one" notification to be consumed by a waiter on blockUntilIdleForTest?
> 
> In general I'm not sure whether the `notify_one` optimization is worth the correctness risk as the code evolves.
Ah, you are right, a thread waiting for done might catch that one as well, but I think it only applies to that one. Is there a possibility of `blockUntilIdleForTest` and `enqueue` being called from different threads?

There is still the argument of code evolution, but I don't think we should ever end up in a state in which an enqueue and a wait that will not consume that enqueue should occur concurrently.


================
Comment at: clangd/index/Background.h:80
+  // Must be last, spawned thread reads instance vars.
+  llvm::SmallVector<std::thread, 8> ThreadPool;
 };
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > Why not `std::vector`? Memory allocs won't ever be a bottleneck here.
> ilya was saying nice things about `llvm::ThreadPool` recently - worth a look?
going for llvm::ThreadPool


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53651