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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Update from the offline meeting: decided to start with `llvm::ThreadPool` for background tasks and lower thread priorities for background tasks.



================
Comment at: clangd/index/Background.cpp:89
   }
-  QueueCV.notify_all();
+  QueueCV.notify_one();
 }
----------------
kadircet wrote:
> 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.
> 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.

We definitely will, +1 to Sam's comment, please call `notify_all` here. It's too easy to deadlock with `notify_one`.
E.g. just from the top of my mind, we can easily start adding tasks from a different thread if we start watching for changes (to compile commands, to header dependencies, etc.)



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53651