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