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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/Threading.cpp:102
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+  sched_param priority;
----------------
don't you also need to actually include it?


================
Comment at: clangd/Threading.h:120
+
+enum ThreadPriority {
+  LOW = 0,
----------------
nit: enum class since this is at clangd scope


================
Comment at: clangd/Threading.h:121
+enum ThreadPriority {
+  LOW = 0,
+  NORMAL = 1,
----------------
nit: llvm tends to spell enum values like `Low`, `Normal`


================
Comment at: clangd/Threading.h:124
+};
+void setThreadPriority(std::thread &T, ThreadPriority Priority);
+
----------------
This deserves a comment - in particular that it may be a no-op.


================
Comment at: clangd/index/Background.cpp:35
+  assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+  while(ThreadPoolSize--) {
+    ThreadPool.emplace_back([this] { run(); });
----------------
nit: clang-format


================
Comment at: clangd/index/Background.cpp:37
+    ThreadPool.emplace_back([this] { run(); });
+    setThreadPriority(ThreadPool.back(), ThreadPriority::LOW);
+  }
----------------
we discussed having different priority tasks, which would require priorities to be set up differently (to avoid workers never waking up to consume high priority tasks).

OK to leave this for now, but deserves a comment.


================
Comment at: clangd/index/Background.h:80
+  // Must be last, spawned thread reads instance vars.
+  llvm::SmallVector<std::thread, 8> ThreadPool;
 };
----------------
kadircet wrote:
> 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
Any problems with llvm::ThreadPool?


================
Comment at: clangd/index/Background.h:19
 #include "llvm/Support/SHA1.h"
+#include "TUScheduler.h"
 #include <condition_variable>
----------------
depending on TUScheduler doesn't make sense here. Move the function to Threading.h instead?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53651