[PATCH] D42573: [clangd] The new threading implementation
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 5 14:30:53 PST 2018
sammccall added a comment.
Very nice! Thanks for adding the docs to TUScheduler implementation, makes a big difference.
Rest is almost all readability/comment bits. Substantive stuff is:
- getUsedBytes() looks racy
- I'm not sure we're choosing the right preamble
My understanding is the threading-related bits of CppFile (locking, and deferRebuild etc) are obsolete after this patch.
It's fine not to delete them here (I guess there could be some fallout), but maybe you want to add a comment in this patch marking them as obsolete?
Testing: I think this is mostly covered by the existing TUScheduler tests. I'd suggest adding a unit test to AsyncTaskRunner though: e.g. start a bunch of threads while holding a mutex that prevents them from making progress (to verify actually async), then have them increment a counter and verify that the counter is N after waitForAll() returns.
================
Comment at: clangd/ASTWorker.cpp:102
+ // not waste time on it.
+ LastUpdateCF->cancel();
+ }
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > This strategy has some upsides:
> > - we eventually get diagnostics for the latest version
> > - we don't begin building an AST that we know can never be read
> > - we don't "wait" when there's something useful to do
> > - it's simple to implement (apart from cancellation)
> > And downsides:
> > - diagnostics are built on the first keystroke
> > - indexing blocks interactive actions, because we don't (and can't reasonably) respect cancellation before calling OnUpdated
> > - it requires CancellationFlag even though our actions aren't cancelable
> >
> > What's the goal here - is this a strategy to keep? Or a placeholder? Or trying to maximize compatibility with the previous code?
> Trying to maximize the compatibility with existing code. There are certainly other strategies to schedule updates.
> I'm perfectly happy to explore those, but would try to keep it out of this patch, it's touching quite a lot of things already. And we should probably remove the callback from update in order to make implementing the timeouts simpler
This sounds fine - can you add a little bit of this rationale and things we might want to change? Maybe at the end of the header comment?
As it stands, it would be very easy for us to land this, move onto other things for a month, and lose the reasoning why the strategy is this way.
================
Comment at: clangd/TUScheduler.cpp:38
+ // Wait for all in-flight tasks to finish.
+ Tasks.waitForAll();
+}
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > destructor will do this
> It's much safer to call it explicitly to not depend no the order of fields in the class.
Hmm, not sure it's much safer than putting a comment on a member, but up to you.
If we're not using the destructor-blocking behavior, remove the destructor or replace with an assert?
================
Comment at: clangd/TUScheduler.cpp:9
+//===----------------------------------------------------------------------===//
+// For each file, managed by TUScheduler, we store a single ASTWorker that
+// manages an AST for that file. All operations that modify or read the AST are
----------------
nit: store -> create? storage is a bit more complicated, but doesn't need to clutter the opening sentence.
================
Comment at: clangd/TUScheduler.cpp:11
+// manages an AST for that file. All operations that modify or read the AST are
+// run on a separate dedicated thread asynchronously in FIFO order. The
+// ASTWorker that manages the AST is owned both by the processing thread and the
----------------
nit: blank line after "...FIFO order"? separating "this is the hierarchy" from "this is how we manage lifetimes".
In fact, I'd consider moving the whole lifetime explanation down and merging it with the ASTWorker class comment. It's more detail than strategy.
================
Comment at: clangd/TUScheduler.cpp:12
+// run on a separate dedicated thread asynchronously in FIFO order. The
+// ASTWorker that manages the AST is owned both by the processing thread and the
+// TUScheduler. Therefore the destructor of ASTWorker to not block, as it is
----------------
is shared by?
================
Comment at: clangd/TUScheduler.cpp:13
+// ASTWorker that manages the AST is owned both by the processing thread and the
+// TUScheduler. Therefore the destructor of ASTWorker to not block, as it is
+// always run after the processing loop is finished. When the TUScheduler gives
----------------
I can't really follow these two sentences. I think we may want to give a little motivation (it's not clear why the destructor needs to not block) before introducing ASTWorkerHandle. e.g.
// The TUScheduler should discard an ASTWorker when remove() is called, but its thread may
// be busy and we don't want to block. So the workers are accessed via an ASTWorkerHandle.
// Destroying the handle signals the worker to exit its run loop, gives up shared ownership of the worker, and detaches the thread.
================
Comment at: clangd/TUScheduler.cpp:82
+ const std::shared_ptr<CppFile> File;
+ // Inputs, corresponding to the current state of File.
+ ParseInputs FileInputs;
----------------
ah, this is my confusion - i mixed up "current state of File" with "current state of the file", which are almost opposites!
Do you think it's confusing to call the `File` member `AST`? All the callsites actually seem to make sense with that name.
================
Comment at: clangd/TUScheduler.cpp:84
+ ParseInputs FileInputs;
+ mutable std::mutex Mutex;
+ // Set to true to signal run() to finish processin
----------------
Not totally obvious in this class what the mutex is for - consider a comment.
e.g. `// Guards members used by both TUScheduler and the worker thread.`
================
Comment at: clangd/TUScheduler.cpp:85
+ mutable std::mutex Mutex;
+ // Set to true to signal run() to finish processin
+ bool Done; /* GUARDED_BY(Mutex) */
----------------
processing.
================
Comment at: clangd/TUScheduler.cpp:136
+ : RunSync(RunSync), Barrier(Barrier), File(std::move(File)), Done(false) {
+ if (RunSync)
+ return;
----------------
no-op.
(If it's just an initlist, inline?)
================
Comment at: clangd/TUScheduler.cpp:159
+ auto Diags = File->rebuild(std::move(Inputs));
+ OnUpdated(std::move(Diags));
+ };
----------------
This deserves a comment for why we don't check isCancelled again... (concretely I guess, why we need it to actually get diagnostics).
================
Comment at: clangd/TUScheduler.cpp:162
+
+ if (RunSync) {
+ Task(CancellationFlag(), std::move(OnUpdated));
----------------
It'd separate the work from the mechanism a bit more to pull out:
startTask(task, isUpdate, args...):
if runSync
task(args...)
else
grab context, bind args, push request, notify
it looks like all cases fit?
================
Comment at: clangd/TUScheduler.cpp:186
+ auto AST = File->getAST().get();
+ AST->runUnderLock([&](ParsedAST *AST) {
+ if (!AST) {
----------------
why under lock? I thought this is the only thread that can access?
(if this locking is a no-op pending cleanup, fixme)
================
Comment at: clangd/TUScheduler.cpp:217
+std::size_t ASTWorker::getUsedBytes() const {
+ std::lock_guard<std::mutex> Lock(Mutex);
+ return File->getUsedBytes();
----------------
I don't think this works - other accesses of File aren't guarded by the mutex?
(And File is documented as only being used by the worker thread)
One option is to cache this in a member after each rebuild(), and put that member behind the mutex. It won't catch ASTs that grow over time, though :-(
A fundamental question is what this function should do if called while the AST is being rebuilt on the worker thread.
================
Comment at: clangd/TUScheduler.cpp:236
+ RequestsCV.wait(Lock, [&]() { return Done || !Requests.empty(); });
+ if (Requests.empty()) {
+ assert(Done);
----------------
So when `Done && !Requests.empty()`, we complete the queue even though the file is removed.
This seems like the right behavior, but deserves a line-comment.
================
Comment at: clangd/TUScheduler.cpp:251
+
+ASTWorkerHandle::ASTWorkerHandle(std::shared_ptr<ASTWorker> Worker)
+ : Worker(std::move(Worker)) {
----------------
nit: inlining these trivial methods might aid readability
================
Comment at: clangd/TUScheduler.cpp:285
+struct TUScheduler::FileData {
+ FileData(ParseInputs Inputs, ASTWorkerHandle Worker)
+ : Inputs(std::move(Inputs)), Worker(std::move(Worker)) {}
----------------
any need for this constructor?
================
Comment at: clangd/TUScheduler.cpp:288
+
+ ParseInputs Inputs;
+ ASTWorkerHandle Worker;
----------------
This could use a comment and/or a more descriptive variable name to distinguish it from Worker.FileInputs.
================
Comment at: clangd/TUScheduler.cpp:301
+TUScheduler::~TUScheduler() {
+ // Clear all FileData objects to notify all workers that they need to stop.
+ Files.clear();
----------------
nit: just "Notify all workers that they need to stop", rather than echoing the code
================
Comment at: clangd/TUScheduler.cpp:312
OnUpdated) {
- CachedInputs[File] = Inputs;
-
- auto Resources = Files.getOrCreateFile(File);
- auto DeferredRebuild = Resources->deferRebuild(std::move(Inputs));
+ auto It = Files.find(File);
+ if (It == Files.end()) {
----------------
Maybe clearer than STL it->second stuff:
FileData &FD = Files[File]
if (!FD.Worker)
FD.Worker = Create(...);
FD.Inputs = move(Inputs)
================
Comment at: clangd/TUScheduler.cpp:327
- Threads.addToFront(
- [](decltype(OnUpdated) OnUpdated,
- decltype(DeferredRebuild) DeferredRebuild) {
- auto Diags = DeferredRebuild();
- OnUpdated(Diags);
- },
- std::move(OnUpdated), std::move(DeferredRebuild));
+ It->second->Worker->update(Inputs, std::move(OnUpdated));
}
----------------
this looks like a second copy of Inputs, we should only need one
================
Comment at: clangd/TUScheduler.cpp:365
- const ParseInputs &Inputs = getInputs(File);
+ ParseInputs InputsCopy = It->second->Inputs;
std::shared_ptr<const PreambleData> Preamble =
----------------
why do we want to make this copy rather than *only* getting the preamble within the task?
Is it to do with preferring preambles from the past, but accepting those from the future?
If I'm understanding right, the general case is these versions:
- V1: this preamble has finished building before `runWithPreamble` is called
- V2: this update() came before `runWithPreamble`, but wasn't built yet. It finished building before the read task ran.
- V3: this update() came before `runWithPreamble`, but wasn't built by the time the read task ran.
- V4: this update() came after `runWithPreamble` was called, but finished before the read task ran (semaphore race?)
- V5: this came after `runWithPreamble` and finished after the read task
Ideally I guess we'd prefer V2, falling back to V1 -> V4 -> null in that order.
Just calling `getPossiblyStalePreamble()` in the task would return V4 -> V2 -> V1 -> null.
The current code will return V1 -> V4 -> V2.
But we have to get really unlucky for V4 to exist, whereas V2 is quite likely to exist I think. This means that only calling `getPossiblyStalePreamble()` once, in the task, seems more likely to get this right to me.
================
Comment at: clangd/TUScheduler.h:100
+ llvm::StringMap<std::unique_ptr<FileData>> Files;
+ AsyncTaskRunner Tasks;
};
----------------
Can we make this an `optional<AsyncTaskRunner>`, empty if we're running synchronously?
That way we'll get an assert if we try to spawn threads in the wrong mode.
I guess RunSync can go away then.
(And Worker::Create() takes a pointer I guess)
================
Comment at: clangd/TUScheduler.h:90
+ const bool StorePreamblesInMemory;
+ const std::shared_ptr<PCHContainerOperations> PCHs;
+ const ASTParsedCallback ASTCallback;
----------------
nit: this name is pervasive, but it stopped me understanding what these are - PCHOps?
================
Comment at: clangd/Threading.h:67
+
+ void waitForAll();
+ void runAsync(UniqueFunction<void()> Action);
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > any need to expose this separately from the destructor?
> It's a useful helper to have.
> We can use it to rewrite tests when removing `future` returned from `addDocument`/`removeDocument`.
> Also see my other comment about calling it in destructor of `TUScheduler`.
Yep, the testing use case makes sense. Let's replace the dtor with an assert though, if we don't think owning this object is a safe way to guard.
================
Comment at: clangd/Threading.h:27
+/// Once cancelled, cannot be returned to the previous state.
+/// FIXME: We should split this class it into consumers and producers of the
+/// cancellation flags.
----------------
hmm, I'm not sure there's enough value in this FIXME to make it ever worth doing. I think it will live forever and we should just drop it :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42573
More information about the cfe-commits
mailing list