[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
Tue Mar 31 09:22:26 PDT 2020
sammccall added a comment.
Had some discussion offline.
- having ASTWorker not worry about preamble validity before dispatching to preambleworker seems like a win
- for this, preambleworker needs to call preambleReady whether it's new or not, so ASTWorker can produce diagnostics
- AST reuse from diagnostics->request seems much more useful than the other way around (e.g. it reduces request latency), so don't bother with the latter. (And we can drop diagnostic computation in some cases)
This yields pseudocode like:
ASTWorker::update(): enqueue({
currentInputs = inputs
preambleworker::update(inputs)
})
ASTWorker::runWithAST(): enqueue({
ast = cache.get()
if (!ast) patch preamble and build ast
action(ast)
cache.put(ast)
})
PreambleWorker::update(): enqueue({
if (!preamble.compatible(inputs))
build preamble
ASTWorker::preambleReady(preamble)
})
ASTWorker::preambleReady(): enqueue({
preamble = p
build ast
publish ast.diagnostics
if (inputs == currentInputs) cache.put(ast)
else if (preamble != oldPreamble) cache.get() // force next read to use this preamble
})
(I'm not sure how simple the actual code can be. I do think defining the methods in that order may help readability)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:8
//===----------------------------------------------------------------------===//
-// For each file, managed by TUScheduler, we create a single ASTWorker that
-// 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.
+// TUScheduler stores a worker per active file. This worker is called ASTWorker
+// and manages updates(modifications to file contents) and reads(actions
----------------
nit: just "This ASTWorker manages updates..."
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:8
//===----------------------------------------------------------------------===//
-// For each file, managed by TUScheduler, we create a single ASTWorker that
-// 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.
+// TUScheduler stores a worker per active file. This worker is called ASTWorker
+// and manages updates(modifications to file contents) and reads(actions
----------------
sammccall wrote:
> nit: just "This ASTWorker manages updates..."
uber-nit: I'd say the scheduler *manages* workers and the worker *processes* updates.
(Just to avoid mixing metaphors)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:9
+// TUScheduler stores a worker per active file. This worker is called ASTWorker
+// and manages updates(modifications to file contents) and reads(actions
+// performed on preamble/AST) to the file.
----------------
nit: space before open parens (not a big deal, but occurs several times)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:16
//
-// 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.
+// An update request changes latest inputs to ensure any subsequent read sees
+// the version of the file they were requested. In addition to that it might
----------------
changes latest inputs -> replaces the current parser inputs
("changes" suggests some tricky mutation, and inputs isn't defined here)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:17
+// An update request changes latest inputs to ensure any subsequent read sees
+// the version of the file they were requested. In addition to that it might
+// result in publishing diagnostics.
----------------
You need to mention "building an AST" here, as you reference it below.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:25
//
-// Rationale for cancelling updates.
-// LSP clients can send updates to clangd on each keystroke. Some files take
-// significant time to parse (e.g. a few seconds) and clangd can get starved by
-// the updates to those files. Therefore we try to process only the last update,
-// if possible.
-// Our current strategy to do that is the following:
-// - For each update we immediately schedule rebuild of the AST.
-// - Rebuild of the AST checks if it was cancelled before doing any actual work.
-// If it was, it does not do an actual rebuild, only reports llvm::None to the
-// callback
-// - When adding an update, we cancel the last update in the queue if it didn't
-// have any reads.
-// There is probably a optimal ways to do that. One approach we might take is
-// the following:
-// - For each update we remember the pending inputs, but delay rebuild of the
-// AST for some timeout.
-// - If subsequent updates come before rebuild was started, we replace the
-// pending inputs and reset the timer.
-// - If any reads of the AST are scheduled, we start building the AST
-// immediately.
+// ASTWorker processes the file in two parts, a preamble and a mainfile
+// section. A preamble can be reused between multiple versions of the file if it
----------------
nit: we use "main file" or "main-file" much more often than "mainfile".
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:26
+// ASTWorker processes the file in two parts, a preamble and a mainfile
+// section. A preamble can be reused between multiple versions of the file if it
+// isn't invalidated by a modification to a header, compile commands or
----------------
it it isn't -> until?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:29
+// modification to relevant part of the current file. Such a preamble is called
+// usable.
+// In the presence of stale(non-usable) preambles, ASTWorker won't publish
----------------
I don't think "usable" is a good name for this, because we *use* preambles that are not "usable".
I think "compatible" or "up-to-date" or "fresh" or so would have enough semantic distance to make this less confusing.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:37
+// invalidated by an update request, a new build will be requested on
+// PreambleThread. PreambleThread serves those requests by building preambles on
+// a dedicated thread. Since PreambleThread only receives requests for newer
----------------
Nit: first and third sentences in this paragraph say the same thing, drop the third?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:43
+//
+// Whenever PreambleThread finishes a build, ASTWorker will enqueue a task to
+// build a new AST using that preamble, called golden AST to publish
----------------
This gets a bit far into implementation details (uses of queues etc) and is hard to follow. Maybe:
When a new preamble is built, a "golden" AST is immediately built from that version of the file.
This ensures diagnostics get updated even if the queue is full.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:48
+// ASTWorker queue is full.
+// These golden ASTs also ensures diagnostics are always for newer versions of
+// the file. As diagnostics are only published for update requests after a
----------------
This is talking about the subtleties of ordering/version guarantees - not really interesting from the outside, and not enough detail here to avoid reading the code. I'd suggest moving these to preambleReady or so.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:233
+ std::vector<Diag> CIDiags, WantDiagnostics WantDiags) {
// Make possibly expensive copy while not holding the lock.
+ Request Req = {std::make_unique<CompilerInvocation>(CI), std::move(PI),
----------------
Some params get copied explicitly, others get passed by value. Pick one strategy?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:235
+ Request Req = {std::make_unique<CompilerInvocation>(CI), std::move(PI),
+ std::move(CIDiags), std::move(WantDiags),
+ Context::current().clone()};
----------------
nit: WantDiags is an enum, no move
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:254
void run() {
- dlog("Starting preamble worker for {0}", FileName);
+ dlog("PreambleThread: Starting worker for {0}", FileName);
while (true) {
----------------
The prefix "PreambleThread" implies that there's something in the code called PreambleThread, and that there's one such thing, neither are true.
(I also think there's a risk in adding ad-hoc prefixes to things that it adds noise and becomes less readable to people with little context, though at dlog that's less of an issue)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:322
- /// 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.
- /// FIXME: We shouldn't cache failed preambles, if we've got a successful
- /// build before.
- void build(Request Req) {
- assert(Req.CI && "Got preamble request with null compiler invocation");
- const ParseInputs &Inputs = Req.Inputs;
- std::shared_ptr<const PreambleData> OldPreamble =
- Inputs.ForceRebuild ? nullptr : latest();
-
- Status.update([&](TUStatus &Status) {
- Status.PreambleActivity = PreambleAction::Building;
- });
-
- auto Preamble = clang::clangd::buildPreamble(
- FileName, std::move(*Req.CI), OldPreamble, Inputs, StoreInMemory,
- [this, Version(Inputs.Version)](
- ASTContext &Ctx, std::shared_ptr<clang::Preprocessor> PP,
- const CanonicalIncludes &CanonIncludes) {
- Callbacks.onPreambleAST(FileName, Version, Ctx, std::move(PP),
- CanonIncludes);
- });
- {
- 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);
- }
- }
+ /// Builds a preamble for \p Req will reuse LatestBuild if possible. Notifies
+ /// ASTWorker.
----------------
this sentence doesn't parse :-)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:341
SynchronizedTUStatus &Status;
+ ASTWorker &AW;
};
----------------
Please don't use initialisms like this for members, they're hard to follow out of context.
I'd suggest ASTPeer (and PreamblePeer) or just Peer, to indicate these threads are tightly-coupled partners.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:402
+ /// Used to inform ASTWorker about a new preamble build by PreambleThread.
+ void preambleReady(std::unique_ptr<CompilerInvocation> CI, ParseInputs PI,
+ std::shared_ptr<const PreambleData> Preamble,
----------------
nit: move somewhere more appropriate (near getCurrentPreamble or update if modelling as public, else into the private section?)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:402
+ /// Used to inform ASTWorker about a new preamble build by PreambleThread.
+ void preambleReady(std::unique_ptr<CompilerInvocation> CI, ParseInputs PI,
+ std::shared_ptr<const PreambleData> Preamble,
----------------
sammccall wrote:
> nit: move somewhere more appropriate (near getCurrentPreamble or update if modelling as public, else into the private section?)
nit: use a verb for mutating operations (updatePreamble)?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:409
+ /// cached one if applicable. Assumes LatestPreamble is usable by \p Inputs.
+ void publishDiagnostics(std::unique_ptr<CompilerInvocation> Invocation,
+ ParseInputs Inputs, std::vector<Diag> CIDiags);
----------------
This name is confusing, almost all of its implementation is building AST.
generateDiagnostics?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:481
std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
+ std::queue<Request> ReceivedPreambles; /* GUARDED_BY(Mutex) */
llvm::Optional<Request> CurrentRequest; /* GUARDED_BY(Mutex) */
----------------
PreambleRequests or GoldenASTRequests or so? The queue doens't actually contain preambles.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:541
+void PreambleThread::build(Request Req) {
+ assert(Req.CI && "Got preamble request with null compiler invocation");
----------------
Placing this between ASTWorker and its implementation seems confusing - move it down?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:554
+ LatestBuild->Version, Inputs.Version, FileName);
+ // We still notify the ASTWorker to make sure diagnostics are updated to
+ // the latest version of the file without requiring user update.
----------------
This is going to trigger a rebuild of the fileindex - is it really necessary?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:630
+ WantDiagnostics WantDiags) {
+ std::string TaskName = llvm::formatv("Preamble Update ({0})", PI.Version);
+ // Store preamble and build diagnostics with new preamble if requested.
----------------
"Golden AST from preamble"?
Current text is not terribly descriptive...
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:644
+ }
+ // Give up our ownership to old preamble before starting expensive
+ Preamble.reset();
----------------
incomplete comment
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:675
+ assert(LatestPreamble);
+ assert(isPreambleUsable(*LatestPreamble, Inputs, FileName, *Invocation));
+
----------------
this isn't a safe assert - it does IO and could transiently become true/false
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