This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Refactored threading in ClangdServer

Authored by ilya-biryukov on Jan 17 2018, 6:16 AM.



We now provide an abstraction of Scheduler that abstracts threading
and resource management in ClangdServer.
No changes to behavior are intended with an exception of changed error
This patch is preliminary work to allow a revamped threading
implementation that will move the threading code out of CppFile.

Diff Detail


Event Timeline

ilya-biryukov created this revision.Jan 17 2018, 6:16 AM

Here's also a combined diff from both this and child revision: D42177

bkramer added inline comments.Jan 17 2018, 7:32 AM
107 ↗(On Diff #130163)

What's so simple about it? Why not clangd::ThreadPool?

Also there's llvm::ThreadPool, what's the difference between them?

ilya-biryukov added inline comments.Jan 17 2018, 9:25 AM
107 ↗(On Diff #130163)

Will rename it to ThreadPool.
Differences are:

  • llvm::ThreadPool always process requests in FIFO order, we allow LIFO here (for code completion).
  • llvm::ThreadPool will run tasks synchronously when LLVM_ENABLE_THREADS is set to 0. I'm not sure that makes sense for clangd, which has a runtime switch for that (-run-synchronously flag)
  • llvm::ThreadPool will not process any tasks when ThreadsCount is set to 0, our implementation processes the tasks synchronously instead.

I'll also be adding per-unit queues in the latter commit (aka thread affinity) to our thead pool, so it'll have more differences. I suggest waiting a day or two before I send the patch for review.

Another minor difference is:

  • llvm::ThreadPool creates a std::packaged_task and std::future for each task, our implementation simply runs the provided actions. The latter means less book-keeping and is more efficient, but I don't think it matters.

So, I simultaneously think this is basically ready to land, and I want substantial changes :-)

This is much better already than what we have, and where I think we can further improve the design, this is a natural point on the way.

My preferred next steps would be:

  • we discuss a little bit the directions I've outlined below, without the assumption that they belong in this patch.
  • to the extent you agree, you make the less-invasive changes here (e.g. adding a VersionConstraint API, but only actually supporting the cases you've implemented)
  • once we're on the same page for the other stuff, I'm happy to pick up any amount of it myself - whatever's not going to step on your toes
37 ↗(On Diff #130163)

Would be nice to have parallel names to the Scheduler methods, e.g. blockingASTRead() and blockingPreambleRead()

222 ↗(On Diff #130163)

Having trouble with this one.
is this the same as "We currently block the calling thread until the AST is available, to avoid..."?

164 ↗(On Diff #130163)

This is a nice abstraction, so much better than dealing with Cppfile! A couple of observations:

  1. all methods refer to a single file, neither the contracts nor the implementation have any interactions between files. An API that reflects this seems more natural. e.g. it naturally provides operations like "are we tracking this file", and makes it natural to be able to e.g. lock at the per-file level. e.g.

    class WorkingSet { shared_ptr<TranslationUnit> get(Path); shared_ptr<TranslationUnit> getOrCreate(Path) } class TranslationUnit { void update(ParseInputs); void read(Callback); }

This seems like it would make it easier to explain what the semantics of scheduleRemove are, too.

  1. The callbacks from individual methods seem more powerful than needed, and encourage deeper coupling. Basically, the inputs (changes) and outputs (diagnostics, reads) don't seem to want to interact with the same code. This suggests decoupling them: the changes are a sequence of input events, diagnostics are a sequence of output events, reads look much as they do now.

One benefit here is that the versioning relationship between inputs and outputs no longer show up in the function signatures (by coupling an input to a matching output). Expressing them as data makes it easier to tweak them.

  1. It's not spelled out how this interacts with drafts: whether text<->version is maintained here or externally, and what the contracts around versions are. There are no options offered, so I would guess that scheduleUpdate delivers new-version-or-nothing, and scheduleASTRead delivers... current-or-newer? current-or-nothing?

I think it would *probably* be clearer to have versions minted by the external DraftStore, that way we can decouple "we know about the contents of this file" from "we're building this file". e.g. we probably want wildly different policies for discarding resources of old versions, when "resources" = source code vs "resources" = ASTs and preambles.

  1. Scheduler (or anything it decomposes into) is important and isolated enough that it deserves its own header.
169 ↗(On Diff #130163)

this is a performance optimization, right?
I think the scheduler does enough of the compiling that giving it one of its own isn't too wasteful.

173 ↗(On Diff #130163)

This seems like a smell - the command is sometimes an input from callsites that want builds, and sometimes an output to callsites that want builds. Also this class is all about threading, scheduling, and versions, and this method bypasses all of that.

(If this is a hack to keep code complete working while we refactor, that's fine - but the docs should say that and we should know what next steps look like!)

ISTM the issue here is coupling updates to the source with updates to the compile command.
Ultimately we should indeed be compiling a given version with a fixed CC, but that doesn't seem like the clearest interface for callers.

One option is:

    • Scheduler has a reference to the CDB (or a std::function wrapping it), and needs to come up with commands itself
    • it caches commands whenever it can, and has an "invalidateCommand(Path)" to drop its cache. with_file_lock { invalidateCommand(), scheduleUpdate() } is the equivalent to forceRebuild.
  • it provides a scheduleSourceRead which is like scheduleASTRead but provides source, compile command, and the latest preamble available without blocking. This would be used for operations like codecomplete that can't use scheduleASTRead.
184 ↗(On Diff #130163)

The callback here is a bit confusing and YAGNI (both clangd and our known embedders discard the returned future).
It seems enough to return synchronously and know that subsequent reads that ask for = or >= version are going to get nothing back.

200 ↗(On Diff #130163)

can we pass in a consistency policy here? even if we only support a subset of {ast, preamble} x policy for now, a richer API would enable experiments/tradeoffs later. e.g.

enum class VersionConstraint {
  ANY,     // returns the first data that's available, typically immediately
  EQUAL, // returns the data for the previous update
  GREATER_EQUAL, // Returns the data for the previous update, or some subsequent update.

(I guess the semantics you've described for AST read are EQUAL, and ANY for preamble read)

206 ↗(On Diff #130163)

these two names seem a bit confusing - might be easier as just CppFiles, Threads?

The names seem to be ~synonyms for the types, which I don't think is better than echoing the types. (Often there's a completely separate good name, but I don't think so here)

ilya-biryukov marked 4 inline comments as done.
  • Renamed SimpleThreadPool to ThreadPool
  • Removed PCHs from Scheduler's constructor
  • Renamed waitFor(AST|Preamble)Action to blocking(AST|Preamble)Read
  • Updates
  • Remove getLastCommand from Scheduler's interface, pass Inputs to actions instead
ilya-biryukov added inline comments.Jan 23 2018, 10:41 AM
222 ↗(On Diff #130163)

Yes. Used your wording, thanks.

169 ↗(On Diff #130163)

It is probably redundant, yes. Removed it.

173 ↗(On Diff #130163)

I've opted for passing action inputs to the read methods, as discussed offline. The implementation turned out to be ugly, we now store a separate StringMap<ParseInputs> in Scheduler to keep track of the latest inputs and not put that into CppFile.

184 ↗(On Diff #130163)

I'm fine with removing the callback, but I'd like this change to focus on internals of ClangdServer without changing its interface, happy to remove the callback afterwards.
Added a FIXME for that.

200 ↗(On Diff #130163)

I would rather not extend the API beyond what it can do at the moment, but happy to explore this approach after threading gets rid of the legacy stuff that we have now.

206 ↗(On Diff #130163)
  • I'd go with Files instead of CppFiles. WDYT?
  • Executor sounds better to me than Threads. Could you elaborate why you find it confusing?
ilya-biryukov added inline comments.Jan 23 2018, 10:58 AM
164 ↗(On Diff #130163)
  1. Given the nature of the LSP, the users of the interface will probably always call only a single function of TranslationUnit, so we won't win much in terms of the code clarity.
scheduleUpdate(File, Inputs) --> get(File).update(Inputs)

That adds some complexity to the interface, though. I'd opt for not doing that in the initial version.

  1. One place where I find these callbacks useful are tests where we could wait for latest addDocument to complete. A more pressing concern is that the interface of ClangdServer does not allow to remove the callbacks (addDocument returns a future) and I would really love to keep the public interface of ClangdServer the same for the first iteration.
  1. I would err on the side of saying that scheduleASTRead delivers current version. This gives correct results for the current callers of the interface. If we do current-or-newer I'd start with adding the versions to the interface, so that the callers of the API would have enough context to know whether the results correspond to the latest version or not. I'd really love to keep version out of the initial patch, happy to chat about them for follow-up patches.
  1. Totally agree. I planned to move it into a separate header after the changes land. Happy to do that with a follow-up patch.

Agreed we can defer lots of stuff in order to keep this patch compact.
Generally the things I think we can get right before landing:

  • names and file organization for new things
  • documentation including where we want to get to
246 ↗(On Diff #131105)

I think you want to take a reference here, and then capture by value

323 ↗(On Diff #131105)

this fixme doesn't make sense if we're removing the callback

345 ↗(On Diff #131105)

nit: you dropped a move here

400 ↗(On Diff #131105)

nit: "Inp" in InpPreamble is pretty awkward. In this context, Read might work? or IP.

403 ↗(On Diff #131105)

InpPreamble->Preamble->Preamble is pretty hard to understand. Can we find better names for these things? Or at least unpack it on one line?

164 ↗(On Diff #130163)
  1. splitting the functionality up into per-file stuff can definitely be deferred until after this patch. Can you add a FIXME?

It should probably happen before the new implementation though, so it pushes that impl in the right direction.

  1. The right abstraction for tests is probably "wait until the scheduler is idle", I think, and that can be implemented without each of these individual async methods being observable.

Agreed that we should keep ClangdServer's interface the same for this patch, so we can't change this yet. FIXME? (If you agree with the testing strategy, that'd be worth mentioning)

  1. Always delivering exactly the sequenced version is fine, but please spell this out.
  1. I'd like this newly added class to have the right name and file structure when it first lands, rather than trying to move it later.
184 ↗(On Diff #130163)

Sounds good. ScheduleUpdate -> update, though? Since this callback will go away, it's not really observable that anything is being scheduled.

200 ↗(On Diff #130163)

I'm happy for this not to land in this patch, but with some of the other API changes it should go in before/with the new implementation

206 ↗(On Diff #130163)

executor often names an abstraction that's related to but not the same as a threadpool (c.f. java or google3). Not a big deal, but it doesn't seem to add any extra semantics over "threads" which would be less ambiguous.

165 ↗(On Diff #131105)

I'm not sure about so many little structs, vs providing one with some fields being optional.
That would make a unified read API easier. WDYT? we can defer this.

177 ↗(On Diff #131105)

This class has important responsibilities beyond threading itself, which "Scheduler" suggests.

I can't think of a perfectly coherent name, options that seem reasonable:

  • TUManager - pretty bland/vague, but gets what this class is mostly about
  • Workshop - kind of a silly metaphor, but unlikely to be confused with something else
  • Clearinghouse - another silly metaphor, maybe more accurate but more obscure
177 ↗(On Diff #131105)

Worth saying something abouth the threading properties here:

  • Scheduler is not threadsafe, only the main thread should be providing updates and scheduling tasks.
    • callbacks are run on a large threadpool, and it's appropriate to do slow, blocking work in them
195 ↗(On Diff #131105)

similarly scheduleRemove -> remove

202 ↗(On Diff #131105)

I like "schedule" in the description, but it seems to in-the-weeds for a function name - ideally the verb would be what the caller is trying to do, not how we implement it.

runWithAST? or if you want to emphasize the async nature, runSoonWithAST?

216 ↗(On Diff #131105)

duplicate private

ilya-biryukov marked 14 inline comments as done.
  • Adressed review comments.
  • Move threading code to separate files.
ilya-biryukov added inline comments.Jan 26 2018, 2:52 AM
246 ↗(On Diff #131105)

Makes sense, less copies. Thanks.

400 ↗(On Diff #131105)

Used IP

403 ↗(On Diff #131105)

It's now IP->PreambleData->Preamble. Not great, but should make more sense.

164 ↗(On Diff #130163)
  1. I was trying to argue that per-file API might not be the best choice, since it certainly adds complexity both to the interface and to the implementation without changing the behavior too much. Happy to discuss this further.
  2. Added the FIXMEs.
  3. Done.
  4. Done.
184 ↗(On Diff #130163)

update seems good.

165 ↗(On Diff #131105)

Small structs seem better to me. I'd argue they give more type safety, even if it means more typing.
Happy to discuss this in more detail.

177 ↗(On Diff #131105)

Added comments.

177 ↗(On Diff #131105)

It'd be nice to have some mention of the fact that the class handles threading responsibilities. None of the options seem to capture this.
I don't have good suggestions either, though.

202 ↗(On Diff #131105)

runWithAST sounds good. Thanks

ilya-biryukov added inline comments.Jan 29 2018, 5:03 AM
177 ↗(On Diff #131105)

Rename of the Scheduler seems to be the only thing blocking this patch from landing.
I'm happy to go with either of the suggested alternatives or leave as is, I couldn't come up with anything better.

@sammccall, what option would you prefer?

sammccall added inline comments.
177 ↗(On Diff #131105)

For a descriptive name I think we need something TU-related in there. If we want it to cover threads too, I can't think of anything better than TUScheduler.

I'm also happy with Workshop or similar, which prods you to work out what the class actually does. Maybe other people will find this confusing. Will solicit opinions.


Chatted with @ioeric and @hokein - Workshop is too silly. Suggestions were along the lines of what we've been discussing: TUTaskManager or similar.
So I'd probably go with TUScheduler, it's not horrendously long and it's close enough to Scheduler that those of us who spent last week discussing it won't get confused :-)

  • Renamed Scheduler to TUScheduler
  • Use make_scope_exit instead of onScopeExit
  • Consume error in dumpAST

Should be ready now. Will land as soon as the review is accepted.

Can you please remove the threading/ subdirectory?
It seems premature for these two files, and TUScheduler doesn't fit. It's unclear that there will be more.

I'd suggest renaming Threadpool.h -> Threading.h, CancellationFlag might fit in there, though up to you.

1 ↗(On Diff #131926)

this class needs tests

1 ↗(On Diff #131926)

this class needs tests

  • Remove threading/ dir, moved everything to the top-level
  • Rename ThreadPool.h to Threading.h
ilya-biryukov added inline comments.Jan 30 2018, 3:25 AM
1 ↗(On Diff #131926)

Will do :-(

1 ↗(On Diff #131926)

Will do :-(

As discussed offline, basically the only thing to do for testing ThreadPool is to bash it with a workload, and some sort of whole-program stress test seems ideal for this and will also give some coverage to other components (and we should run under tsan!).

TUScheduler on the other hand is a big important class with a clear interface and contract, and is about to get a new implementation - testcases verifying the contracts will be extremely valuable.
These tests don't really need to be concurrency-heavy I think.

10 ↗(On Diff #131941)

header guards are stale, sorry!

10 ↗(On Diff #131941)

this one also stale

  • Properly ignore errors.
  • Run all requests to completion when destroying ThreadPool.
  • Added simple tests for TUScheduler.
  • Fixed include guards.
ilya-biryukov marked 2 inline comments as done.Jan 30 2018, 11:02 PM

All comments should be addressed now. Let me know if I missed anything else.

1 ↗(On Diff #131926)

Added a simple test for it. Please take a look and let me know if you have more ideas on how we should test it.

1 ↗(On Diff #131926)

We have ClangdThreadingTest.StressTest and TUSchedulerTests that both run concurrent operations on ThreadPool.
As you pointed out, this should provide enough coverage for ThreadPool, so I didn't create any extra tests for it.

sammccall accepted this revision.Jan 30 2018, 11:21 PM

Ship it!

37 ↗(On Diff #130163)

Nit: these names got out of sync again

23 ↗(On Diff #132100)

just use llvm::hardware_concurrency()?

30 ↗(On Diff #132100)

add a comment to the destructor saying what it blocks on?

This revision is now accepted and ready to land.Jan 30 2018, 11:21 PM
ilya-biryukov marked 2 inline comments as done.

Addressed last review comments:

  • Rename blockingRead to blockingRun
  • Added a comment to ThreadPool's destructor
ilya-biryukov added inline comments.Jan 31 2018, 12:49 AM
23 ↗(On Diff #132100)

It can return 0, which will cause clangd to run synchronously. This function is only called when someone wants to have at least one worker thread for async processing.

We can change it if you want, but I'd rather leave it as is in this patch.

This revision was automatically updated to reflect the committed changes.