This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Only reduce priority of a thread for indexing.
ClosedPublic

Authored by kadircet on Dec 5 2018, 4:28 AM.

Details

Summary

We'll soon have tasks pending for reading shards from disk, we want
them to have normal priority. Because:

  • They are not CPU intensive, mostly IO bound.
  • Give a good coverage for the project at startup, therefore it is worth spending some cycles.
  • We have only one task per whole CDB rather than one task per file.

Event Timeline

kadircet created this revision.Dec 5 2018, 4:28 AM
ilya-biryukov added inline comments.Dec 5 2018, 6:13 AM
clangd/Threading.h
125

Maybe change the name to setCurrentThreadPrority?
The change itself LG, there does not seem to be a way to get the native handle from std::this_thread...

clangd/index/Background.cpp
200

Maybe make this a parameter of enqueueTask, probably with ThreadPriority::Low being the default?
Seems to be an appropriate place, enqueueTask is a starting point for scheduling and task priority is inherently a property that the scheduler cares about.

kadircet updated this revision to Diff 176834.Dec 5 2018, 8:07 AM
kadircet marked 2 inline comments as done.
  • Move priority change logic into enqueueTask
ilya-biryukov added inline comments.Dec 5 2018, 11:00 AM
clangd/Threading.h
124

The comment looks redundant now. Maybe remove it?

clangd/index/Background.cpp
207–218

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?

kadircet updated this revision to Diff 177007.Dec 6 2018, 10:38 AM
kadircet marked 2 inline comments as done.
  • Delete redundant comment
kadircet added inline comments.Dec 6 2018, 10:41 AM
clangd/index/Background.cpp
207–218

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?

ilya-biryukov added inline comments.Dec 7 2018, 3:50 AM
clangd/index/Background.cpp
207–218

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

217

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?

kadircet updated this revision to Diff 177188.Dec 7 2018, 5:23 AM
kadircet marked 3 inline comments as done.
  • Move priority change logic to runner
  • Change storage for tasks from deque to list, since we only pop from front but we might end up adding into middle.
clangd/index/Background.cpp
207–218

The wrapped callback was rather for getting rid of unnecessary system calls, but since normal tasks come up rarely to queue it doesn't feel like an important thing. Changing it to store the priority in the queue.

ilya-biryukov added inline comments.Dec 13 2018, 3:34 AM
clangd/index/Background.cpp
107–108

NIT: remove braces around a single statement

141

NIT: maybe add newlines around the three lines (setCurrentThreadPriority and Task execution). These are pretty important.

141

Maybe only lower the priority iff it's != Normal. Would avoid system calls in most cases (I believe you mentioned it in the other comment before too)

209

A short comment about the structure of the queue would be in order, i.e. mention that we first store all "normal" tasks, followed by "low" tasks. And that we don't expect the number of "normal" tasks to grow beyond single-digit numbers, so it's ok to do linear search here and insert in that position

211

Maybe change to llvm::find_if(Tasks, [](Task &T) { return T.second == ThreadPriority::Low); } ?
More concise and avoids explicit loop.

kadircet updated this revision to Diff 178052.Dec 13 2018, 6:50 AM
kadircet marked 6 inline comments as done.
  • Address comments
ilya-biryukov added inline comments.Dec 13 2018, 7:57 AM
clangd/index/Background.h
121

Why not std::deque?

kadircet marked an inline comment as done.Dec 13 2018, 8:00 AM
kadircet added inline comments.
clangd/index/Background.h
121

because we can insert in the middle of the queue now, when we skip existing high priority tasks.

ilya-biryukov added inline comments.Dec 13 2018, 9:29 AM
clangd/index/Background.h
121

deque::insert is O(smaller of the distances to either begin or end) and we spend the same time on the linear search anyway.
I believe we can keep it, even though I'm not strictly sure which is more efficient (we certainly don't care)

kadircet updated this revision to Diff 178094.Dec 13 2018, 10:36 AM
kadircet marked 2 inline comments as done.
  • Use std::deque instead of std::list
clangd/index/Background.h
121

Cool, didn't know that. If that's the case we definitely don't care.

ilya-biryukov accepted this revision.Dec 17 2018, 4:20 AM

LGTM

clangd/index/Background.h
121

This rename makes the diff look a bit more complicated than it actually is. I personally like the new name better, but the old name also seemed ok.
Maybe considering keeping the old name to make the diff simpler?

Up to you, though, this does not seem terribly important.

This revision is now accepted and ready to land.Dec 17 2018, 4:20 AM
kadircet updated this revision to Diff 178443.Dec 17 2018, 4:30 AM
kadircet marked an inline comment as done.
  • Revert name from Tasks to Queue
This revision was automatically updated to reflect the committed changes.