[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/index/Background.cpp:202
     std::lock_guard<std::mutex> Lock(QueueMu);
-    Queue.push_back(std::move(T));
+    if (Priority == ThreadPriority::Low) {
+      Queue.push_back(Bind(
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Since we might be interested in scheduling higher-priority tasks first anyway (not in this patch, but still), maybe store a pair of `(Task, Priority)` in the queue and call `setCurrentThreadPriority` when actually running the task?
> I believe we can introduce that later on whenever we have more than 2 priorities, currently we push to front for important tasks and back for the low priority ones. Do you think it is not enough?
Sorry, I somehow missed that `push_front` and `push_back` are used in different cases, I've only noticed the callback wrapping.
I feel storing priorities in the queue produces a more straightforward code as priority is an important scheduling signal, so it fits in nicely there. Using a wrapped callback, OTOH, is a bit hard to read. But that might be a personal preference, up to you.

See the other comment about ordering of the tasks with normal priorities, it looks more important


================
Comment at: clangd/index/Background.cpp:212
+          std::move(T)));
+    } else
+      Queue.push_front(std::move(T));
----------------
It's a bit surprising that we put the new task in front of the other tasks **with a normal priority**.
Do we anticipate a large number of normal priority tasks? I believe our current expectation is that we'll have a single normal task per open compilation database, right? In that case the number of pending normal tasks should be a single-digit number in practice. Therefore, maybe add the new task after all the normal tasks at the start of the queue?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55315/new/

https://reviews.llvm.org/D55315