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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
