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

[Sam McCall via Phabricator via cfe-commits]

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