sammccall added a comment. In https://reviews.llvm.org/D53651#1275517, @ilya-biryukov wrote:
> It's fine to spend one thread spinning on background tasks, but if we're > going to do a threadpool, we should be more careful to not hurt the > performance of foreground tasks. To do that, we should at least: > > - share the semaphore for the number of actively running tasks between > TUScheduler and BackgroundIndex. > - prioritize foreground tasks over background tasks. I don't think I agree with this, at least not without evidence. Can we try thread priorities first? ================ Comment at: clangd/index/Background.cpp:89 } - QueueCV.notify_all(); + QueueCV.notify_one(); } ---------------- 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. ================ Comment at: clangd/index/Background.h:80 + // Must be last, spawned thread reads instance vars. + llvm::SmallVector<std::thread, 8> ThreadPool; }; ---------------- 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? 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
