Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
clangd/ClangdServer.h | ||
---|---|---|
107 ↗ | (On Diff #109493) | Typo: immediately |
Addressed review comments.
- Fixed typos.
- Renamed SchedulingParams to SchedulingOptions.
clangd/ClangdServer.h | ||
---|---|---|
121–122 ↗ | (On Diff #109493) | Currently 1 means: start 1 worker thread to run async operations (that thread is separate from the main thread). |
Great!
clangd/ClangdServer.h | ||
---|---|---|
131 ↗ | (On Diff #109527) | typo: "on separate threads" |
176 ↗ | (On Diff #109527) | typo: "on separate threads" |
clangd/tool/ClangdMain.cpp | ||
24 ↗ | (On Diff #109527) | Extract this somewhere when it can be reused both by this and in clangd/ClangdServer.cpp:90. |
69 ↗ | (On Diff #109527) | Consider inverting this condition. |
clangd/ClangdServer.h | ||
---|---|---|
121–122 ↗ | (On Diff #109493) | As discussed, replaced with unsigned AsyncThreadsCount. Zero means run synchronously. |
It is not an in-place replacement as it does not allow to prioritize new tasks over old ones (new tasks are usually more important for clangd as the old ones are often outdated when new ones are added).
I looked into using llvm::ThreadPool before, but decided to stay with our own implementation.
It gives us more flexibility over threading and it is not hard to maintain (I think it's under 100 lines of code, and the code is rather simple).
Added a test that exercises some multi-threading behaviour.
It really finds bugs, at least it found a problem that was fixed by https://reviews.llvm.org/D36397. (Will make sure to submit that other change before this one).
As discussed with @klimek, will move to llvm::ThreadPool after submitting a fix for it to allow functions with moveable-only arguments to be used there (that's what clangd uses).