[PATCH] D94875: [clangd] ignore parallelism level for quick tasks

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

> Save a file, trigger format-on-save, which hangs because clangd is busy

OK, that's really terrible :-( In some way this is LSP's fault, the whole design is around async unreliable language servers, and then they put in a couple of features that really have to be sync.
But those features solve real problems...
We're really lucky here that formatting doesn't need an AST or preamble, there's no real fundamental reason for that.

I was pretty skeptical of the implementation at first, but working through the alternatives brought me around.

---

(Rest is my notes, feel free to ignore)

1. The technique in the patch: separate pool for cheap tasks with short deadlines. This gets slow if the tasks don't turn out to be cheap.
2. Or we could take them off the semaphore altogether. This spawns lots of threads if the tasks don't turn out to be cheap.
3. Another option is to lean on the fact that the task in question here isn't terribly important - we'd rather give up than queue. We could do this by using `try_lock` instead of `lock` on the semaphore. This degrades functionality if we get overlapping requests.
4. We could record priorities and run the highest-priority task next.

4 is a non-starter without pre-emption: we have to wait for a task to finish, and probably all running tasks are slow.

3 trades away reliability for latency guarantees. I don't like it because it may do so even below the threshold where latency matters.

We could combine 1+3: a separate fast-tasks semaphore, but try_lock. This will still format under load, but will refuse to ever form a queue.
I guess the most relevant scenario is some large save-all operation. Still, we don't have any evidence this is going to be slow enough that we should choose to be unreliable.

1 vs 2 is partly a question of how you want to fail. Queueing seems just as good as leaking threads to me.
But it also determines whether we get any parallelism in a large-save-all scenario. It seems a little silly not to, to me - so maybe we should increase the semaphore size.



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1251
                           : std::make_unique<ParsingCallbacks>()),
-      Barrier(Opts.AsyncThreadsCount),
+      Barrier(Opts.AsyncThreadsCount), QuickRunBarrier(1),
       IdleASTs(
----------------
1 means if we get *any* concurrent requests they'll execute serially instead of in parallel.

I get the desire to limit this, and the idea that the tasks should be fast, but this seems like it'll be the bottleneck if the user hits "save all" after a rename hits 20 files in their editor.

We might as well set it to 4 or AsyncThreadsCount or something?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1328
+
+void TUScheduler::quickRun(llvm::StringRef Name, llvm::StringRef Path,
+                           llvm::unique_function<void()> Action) {
----------------
nit: i'd slightly prefer `runQuick`, as it groups alphabetically with the other methods.

And that way "quick" is jammed ambiguously between the "run" and the "action", and applies to both :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94875