[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 26 09:45:51 PDT 2020
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:8
//===----------------------------------------------------------------------===//
// For each file, managed by TUScheduler, we create a single ASTWorker that
+// manages an AST and a preamble for that file. All operations that modify or
----------------
This is explaining a pretty complicated thing, so I think it's particularly important to clearly organize the explanation, use consistent terminology, and avoid including unneccesary details.
I'd suggest introducing with paragraphs in this order:
- TUScheduler and the idea of a worker per TU.
- ASTWorker thread, the queue of interleaved updates and reads, elision of dead updates.
- Compatibility of preambles and updates, picky reads.
- PreambleWorker thread, elision of preambles, golden ASTs.
- Published ASTs, epochs/monotonicity.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:11
+// read the AST are run on a separate dedicated thread asynchronously in FIFO
+// order. There is a second thread for preamble builds, ASTWorker thread issues
+// updates on that preamble thread as preamble becomes stale.
----------------
nit: name the other thread (there is a second PreambleWorker thread ...)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:12
+// order. There is a second thread for preamble builds, ASTWorker thread issues
+// updates on that preamble thread as preamble becomes stale.
+//
----------------
"issues updates" is vague. What about "... enqueues rebuilds on the PreambleWorker thread as preamble becomes stale. Currently, it then immediately blocks on that request."
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:19
+// FIXME: Get rid of the synchronization between ASTWorker::update and
+// PreambleThread builds by using patched ASTs instead of compatible preambles
+// for reads. Only keep the blocking behaviour for picky read requests, e.g.
----------------
You haven't defined "compatible" anywhere, and it's not obvious what it means.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:23
+//
+// Updates are processed in two phases, by issuing a preamble and an ast build.
+// The preamble build gets issued into a different worker and might be
----------------
The wording here (particularly the word "phases") implies sequencing: an update results in a preamble build followed by an ast build in sequence, when in fact it usually results in an ast build and preamble build in parallel (the former usually finishing first) with a second ast build sequenced after the preamble build.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:25
+// The preamble build gets issued into a different worker and might be
+// overwritten by subsequent updates. An AST will be built for an update if
+// latest built preamble is compatible for it or a new preamble gets built for
----------------
This isn't true, an AST is built for an update if it is needed (a read is based on it, wantdiagnostics=yes, it changed the preamble, or wantdiagnostics=maybe and the debounce expired)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:29
+// AST builds are always executed on ASTWorker thread, either immediately
+// following an update request with a compatible preamble, or through a new
+// request, inserted at front of the queue, from PreambleThread after building a
----------------
I'm not sure what compatible means here, but we will certainly build ASTs for incompatible preambles.
(Or is this describing the intermediate state as of this patch, with the plan to rewrite it all next patch? I think we should rather describe the new model, and then document the current deviations from it)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:29
+// AST builds are always executed on ASTWorker thread, either immediately
+// following an update request with a compatible preamble, or through a new
+// request, inserted at front of the queue, from PreambleThread after building a
----------------
sammccall wrote:
> I'm not sure what compatible means here, but we will certainly build ASTs for incompatible preambles.
> (Or is this describing the intermediate state as of this patch, with the plan to rewrite it all next patch? I think we should rather describe the new model, and then document the current deviations from it)
I'm not sure what "immediately following" is relative to: if it's after the update() call, it's not immediate - it has to wait in the queue.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:33
+//
+// Diagnostics and index is updated as a result of building AST for write
+// requests. Progress on those updates are ensured with golden ASTs, as they'll
----------------
"and *the* index *are* updated"... "building *the* AST"
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:33
+//
+// Diagnostics and index is updated as a result of building AST for write
+// requests. Progress on those updates are ensured with golden ASTs, as they'll
----------------
sammccall wrote:
> "and *the* index *are* updated"... "building *the* AST"
you're using "write" and "update" interchangeably here, it would be clearer to be consistent
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:33
+//
+// Diagnostics and index is updated as a result of building AST for write
+// requests. Progress on those updates are ensured with golden ASTs, as they'll
----------------
sammccall wrote:
> sammccall wrote:
> > "and *the* index *are* updated"... "building *the* AST"
> you're using "write" and "update" interchangeably here, it would be clearer to be consistent
I think "as a result of" is just "after"?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:34
+// Diagnostics and index is updated as a result of building AST for write
+// requests. Progress on those updates are ensured with golden ASTs, as they'll
+// be built even when ASTWorker queue is full. Since ASTs aren't built with
----------------
Now we're using "updates" to mean something different...
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:36
+// be built even when ASTWorker queue is full. Since ASTs aren't built with
+// incompatible preambles, we can partition diagnostic updates into epochs of
+// preambles. Each epoch starts with a golden AST(unless that update had
----------------
It's not clear what this sentence is saying: some code is doing this partitioning?
(I know you're describing a property of the output of the process, but it's not clear from the text)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:43
//
// We start processing each update immediately after we receive it. If two or
// more updates come subsequently without reads in-between, we attempt to drop
----------------
I'm not sure why this paragraph is here rather than near the description of the ASTWorker queue.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:47
//
-// The processing thread of the ASTWorker is also responsible for building the
-// preamble. However, unlike AST, the same preamble can be read concurrently, so
-// we run each of async preamble reads on its own thread.
+// ASTWorker is also responsible for serving preambles. Since preambles can be
+// read concurrently, we run each of async preamble reads on its own thread.
----------------
I'm not sure what "serving preambles" means.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:50
//
// To limit the concurrent load that clangd produces we maintain a semaphore
// that keeps more than a fixed number of threads from running concurrently.
----------------
I think this no longer belongs here - merge with TUScheduler::Options::AsyncThreadsCount or just drop
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:53
//
// Rationale for cancelling updates.
// LSP clients can send updates to clangd on each keystroke. Some files take
----------------
This section is basically obsolete - elision of dead updates is (should be) mentioned above, and the details are stale (e.g. it refers to debounce as a possibility) and documented in-line.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:235
public:
+ using PreambleCallback =
+ std::function<void(std::unique_ptr<CompilerInvocation>, ParseInputs,
----------------
This never varies, why use a callback rather than just calling a function on the ASTWorker?
(I think I asked this in the previous version, but didn't see an answer)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:259
std::lock_guard<std::mutex> Lock(Mutex);
- assert(!Done && "Build request to PreambleWorker after stop");
+ // ASTWorker might've updates left in it even after PreambleWorker is
+ // stopped. Ignore those as it should be able to serve remaining requests
----------------
Took me a while to understand what this comment is saying (why is it talking about ASTWorker queue state?)
Maybe:
// If we're shutting down, don't bother building preambles, reads will be cancelled.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:338
/// Builds a preamble for Req and caches it. Might re-use the latest built
/// preamble if it is valid for Req. Also signals waiters about the build.
----------------
this seems to never reuse?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:368
});
- {
- std::lock_guard<std::mutex> Lock(Mutex);
- // LatestBuild might be the last reference to old preamble, do not trigger
- // destructor while holding the lock.
- std::swap(LatestBuild, Preamble);
- }
+ // FIXME: Invalidate NextReq, if this Preamble is compatible with that.
+ Req.PC(std::move(Req.CI), std::move(Req.Inputs), std::move(Preamble));
----------------
That sounds racy to me - what happens if a new request arrives after build() invalidates but before build() returns?
Seems much more robust for PreambleWorker to hold the last built preamble, and ASTWorker to hold the last usable preamble. It's just a shared_ptr, what's the harm :)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:385
+ // Used only for logging.
+ llvm::Optional<std::string> LastInputVersion;
----------------
storing this separately from the latest preamble (i.e. separately from where the decision is made to build/rebuild) seems a bit suspect (not actually wrong, but points towards storing the latest built preamble here too)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:623
std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version);
auto Task = [=]() mutable {
auto RunPublish = [&](llvm::function_ref<void()> Publish) {
----------------
this function is getting too long/monolithic, we should find a way to split it up.
ASTTask should be a member function, I think.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:623
std::string TaskName = llvm::formatv("Update ({0})", Inputs.Version);
auto Task = [=]() mutable {
auto RunPublish = [&](llvm::function_ref<void()> Publish) {
----------------
sammccall wrote:
> this function is getting too long/monolithic, we should find a way to split it up.
> ASTTask should be a member function, I think.
I also lost track of the control flow halfway through the function, and can't work it out.
I don't know really what to concretely advise, but it needs to be a lot simpler, maybe by finding some better abstractions or just better understanding conceptually what this is doing.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:679
}
+ // Task that builds an AST and runs callbacks. Must be run with a preamble
+ // that's compatible with Inputs. Might be run immediately or in future,
----------------
You've called this a task, but it never gets scheduled on a queue, just called in various places. This seems like a helper function, and needs a better name.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:693
+ std::lock_guard<std::mutex> Lock(Mutex);
+ // LatestPreamble might be the last reference to old preamble, do not
+ // trigger destructor while holding the lock.
----------------
Didn't we decide to make this available only after building the golden AST and publishing diagnostics?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76725/new/
https://reviews.llvm.org/D76725
More information about the cfe-commits
mailing list