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