This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Use multiple working threads in clangd.
ClosedPublic

Authored by ilya-biryukov on Aug 3 2017, 1:22 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Aug 3 2017, 1:22 AM
arphaman added inline comments.
clangd/ClangdServer.h
107 ↗(On Diff #109493)

Typo: immediately

klimek added inline comments.Aug 3 2017, 1:38 AM
clangd/ClangdServer.h
105 ↗(On Diff #109493)

I think calling it "Options" is more idiomatic.

110 ↗(On Diff #109493)

Typo: Indicates *that*.

121–122 ↗(On Diff #109493)

Why not: 1 -> run synchronously, > 1, run in parallel?

ilya-biryukov marked 3 inline comments as done.

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).
This makes sense for clangd, as if you do that, you still get code completion that doesn't wait for diagnostics to finish.
On the other hand, it's useful to have -run-synchronously for some tests.

krasimir edited edge metadata.Aug 3 2017, 6:09 AM

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.

  • Removed SchedulingOptions altogether, replaced with AsyncThreadsCount.
ilya-biryukov marked 4 inline comments as done.
  • Fixed more typos/inconsistences in comments, pointed out by @krasimir.
clangd/tool/ClangdMain.cpp
69 ↗(On Diff #109527)

Thanks, good point. I also like non-negated conditions more.
That code was removed in the updated version, though.

ilya-biryukov added inline comments.Aug 7 2017, 9:23 AM
clangd/ClangdServer.h
121–122 ↗(On Diff #109493)

As discussed, replaced with unsigned AsyncThreadsCount. Zero means run synchronously.

klimek edited edge metadata.Aug 8 2017, 1:11 AM

High-level question: Why can't we use llvm::ThreadPool?

High-level question: Why can't we use llvm::ThreadPool?

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 stress test for multithreading.

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).

This revision is now accepted and ready to land.Aug 11 2017, 1:46 AM
This revision was automatically updated to reflect the committed changes.