First step to enable deferred preamble builds. Not intending to land it
alone, will have follow-ups that will implement full deferred build
functionality and will land after all of them are ready.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clangd/TUScheduler.cpp | ||
---|---|---|
155 | This doc comment doesn't mention "thread" once :-) | |
156 | move documentation of requestBuild() to that function? | |
157 | Move documentation of stop() semantics to stop()? | |
159 | we may want to call this PreambleThread? Current name emphasizes peer relationship with ASTWorker, but current code has parent/child relationship. | |
171 | or just update()? | |
201 | I think all the members in this class with "preamble" in the name can drop that word without any loss, e.g. PreambleWorker::waitForFirst() | |
205 | latest()? | |
218 | Why do we rebuild the preamble if stop is requested? | |
265 | as discussed a bit in chat, I think this part needs some more thought. Currently the ASTWorker and PreambleWorker emit data for the same file with some interleaving that's hard to reason about. The state needs an owner. (e.g. consider a class that holds a TUStatus and exposes an Event<TUStatus>, with threadsafe setPreambleAction and setWorkerAction methods, or so) | |
291 | just build(), to avoid confusion clangd::buildPreamble? Also consider merging with updateLatestBuiltPreamble, both are small and private and they always go together. | |
295 | (nullptr should work here?) | |
312 | Yikes, lots of state :-) | |
322 | nit: last vs latest, be consistent | |
325 | please make this owning for simplicity, this isn't a lightweight object anyway | |
592 | this doesn't seem correct (maybe ok in this patch because of the blocking, but not in general). You're assuming the last available preamble is the one that the last AST was built with. I suppose you can't check the preamble of the current ParsedAST because it might not be cached, and you nevertheless want to skip rebuild if the diagnostics are going to be the same. I can't think of anything better than continuing to hold the shared_ptr for PreambleForLastBuiltAST or something like that. |
clang-tools-extra/clangd/TUScheduler.cpp | ||
---|---|---|
218 | it was to be consistent with astworker, but i suppose it isn't necessary as ASTWorker does that to make sure each LSP request gets a reply, and it can be done with stale preambles. Are there any other reasons for ASTWorker to empty the queue after stop? | |
295 | right, it was copied over :/ | |
312 | i like them to be seperate as it makes naming easier :D but i agree that having less state also makes reasoning easier. | |
592 | right, this is just a "hack" to keep this change NFC. in the follow-up patches i am planning to signal whether latest built preamble is reusable for a given ParseInputs, and also signal what the AST should be patched with. diagnostics(ast) will only be built if preamble is re-usable. |
This basically looks good, just nits (mostly about comments, sorry I can't help myself).
I think the only real outstanding thing is TUStatus, which we're still discussing offline. Since this patch is intended to be NFC, I see two paths consistent with the spirit of that:
- work out how to make this emit the same sequence of events (probably in a fairly brittle way), document that, and sort it out in the next patch
- make TUStatus reflect the new model, without changing its essential design or scope. Currently TUStatus is currently (stateful bits about TU, worker thread activity), so the extension would be (stateful bits about TU, worker thread activity, preamble thread activity).
clang-tools-extra/clangd/TUScheduler.cpp | ||
---|---|---|
156 | This sentence is hard to parse (processes, build, requests, and run are all ambiguous between nouns and verbs, and the pronouns, articles etc are missing). Suggestion: "It processes updates asynchronously, building preambles on a dedicated thread. (This thread should be spawned externally and call the run() method)." | |
156 | Actually, is "processing requests" really the clearest way to describe the model here? What about "Whenever the thread is idle and the preamble is outdated, it starts to build a fresh preamble from the latest inputs." | |
157 | nit: "which" appears to refer to RunSync here, and "provided" doesn't exactly mean "true". Suggestion: If RunSync is true, preambles are built synchronously in update() instead. | |
171 | request -> requested version | |
218 | Yeah, I think that's the only reason and the asymmetry is worthwhile here (esp because one preamble is way more expensive than one AST). If that's not clear in the ASTworker piece, please do add a comment! | |
224 | I think the comment adds more confusion than it resolves: you seem to be describing why an alternative (reacquiring the lock after building, and then calling reset?) doesn't work, but never state what that alternative is. Moreover, clearing NextReq after moving the value into CurrentReq seems natural, I'm not sure a comment is necessary (except maybe to complain that std::optional's move constructor doesn't do this!) | |
243 | we've been stopped -> it should stop? | |
300 | If LatestBuild is the last shared_ptr to the old preamble, we're destroying it here with the lock held, which probably does IO. Instead, std::swap(Preamble, LatestBuild) so that the destructor runs when the function exits. | |
310 | (not sure the "written by" comment is needed since this is just a regular guarded-by-mutex?) | |
592 |
can you add a comment about this? (In particular, why it's correct?)
Does "reusable" mean "completely valid" (current semantics), or usable with some tweaks (e.g. added headers)? It would be good to define some precise terminology around this.
SG |
I'd really like to find any way to unblock this that isn't "redesign the API" because I don't think we should do that in a hurry. So also open to other ideas here, or happy to take a stab at this part if it's helpful.
clang-tools-extra/clangd/TUScheduler.cpp | ||
---|---|---|
265 | sent out https://reviews.llvm.org/D76304 | |
412 | it has been moved into PreambleThread instead, same as the one below handling the failure. | |
592 | reusable means completely valid. |
This doc comment doesn't mention "thread" once :-)