This makes addDocument non-blocking and would also allow code completion
(in fallback mode) to run when worker waits for the compile command.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 30530 Build 30529: arc lint + arc unit
Event Timeline
This is long overdue... Thanks!
clangd/TUScheduler.cpp | ||
---|---|---|
346 | The command is filled with a fallback by ClangdServer, right? Why do we need to repeat this here? | |
371 | Could you document that we initially start with a fallback, so update here? | |
372 | Maybe update the heuristic field in the current compile command if its empty to indicate we're using an older command? | |
574 | This is racy, FileInputs is only accessed from a worker thread now. | |
863 | We should add a comment that compile command in inputs is ignored. |
- address review comments
clangd/TUScheduler.cpp | ||
---|---|---|
346 | ASTWorker::FileInputs is not set until ASTWorker runs update and gets compile command. Before that, FileInputs contains empty compile command that can be used by runWithPreamble/runWithAST (afaict). Initializing it to fallback command seems to be a better alternative. | |
372 | If CDB.getCompileCommand failed, we would use Inputs.CompileCommand which should be the fallback command set in ClangdServer. It might be a good idea to use the old command, but it doesn't seem to be in the scope of this patch. Added a FIXME. | |
574 | It looks like we could also guard FileInputs? Adding another variable seems to make the state more complicated. | |
863 | Added commen. Unfortunately, ParseInputs contains other things like Index and Opts, so we couldn't replace it with FS and Contents. |
clangd/TUScheduler.cpp | ||
---|---|---|
346 | That makes sense. Could we remove the getFallbackCommand call from the ClangdServer::addDocument? | |
372 | Oh, I mixed Inputs and FileInputs in my head. | |
574 | FileInputs are currently accessed without locks in a bunch of places and it'd be nice to keep it that way (the alternative is more complicated code). |
- address comments
clangd/TUScheduler.cpp | ||
---|---|---|
346 | Sounds good. | |
574 | The reason I think it would be easier to guard FileInputs with mutex instead of pulling a new variable is that CompileCommand is usually used along with other fields in FileInputs. I think we could manage this with relatively few changes. Please take a look at the new changes. |
clangd/TUScheduler.cpp | ||
---|---|---|
574 | Unfortunately we can't share Inputs.FS safely as the vfs implementations are not thread-safe. |
clangd/TUScheduler.cpp | ||
---|---|---|
574 | It seems to me that the behavior for FS hasn't change in this patch. And FileInputs (incl. Inputs.FS) has always been available/shared across worker threads. We don't seem to have systematic way to prevent raciness before this patch, and it would be good to have it somehow, but it's probably out of the scope. Maybe I'm missing something or misinterpreting. Could you elaborate? |
clangd/TUScheduler.cpp | ||
---|---|---|
574 |
I don't think that's the case, we did not a public API to get the hands on ASTWorker::FileInputs.FS and we're getting one now. The (not very) systematic way to prevent raciness that we use now is to not protect the members which are potentially racy with locks and document they are accessed only from a worker thread. |
clangd/TUScheduler.cpp | ||
---|---|---|
574 | I don't think that makes a fundamental difference as FileInputs.FS is already racy. But if "public" API is the concern, we could simply restrict the scope of the API and make return CompileCommand only. |
clangd/TUScheduler.cpp | ||
---|---|---|
574 | Inside ASTWorker, FileInputs can still be guarded with Mutex as a whole. |
Thanks, the change LG now. Only nits from my side!
clangd/Compiler.h | ||
---|---|---|
19 | NIT: this looks unrelated to the actual change. Maybe revert? | |
clangd/TUScheduler.cpp | ||
228 | Could you add a comment that this is private because Inputs.FS is not thread-safe and the client code should take care to not expose it via a public interface. | |
257 | This comment is not true anymore, the FileInputs might be out-of-sync with the AST for short spans of time. /// File inputs, currently being used by the worker. /// Inputs are written and read by the worker thread, compile command can also be consumed by clients of ASTWorker. | |
347 | NIT: maybe add a comment explaining why only CompileCommand is set and not the other fields? // Other fields are never read outside worker thread and the worker thread will initialize them before first use. | |
367 | IIUC, The comment does not correspond to the latest version. | |
387 | NIT: maybe avoid using new? FileInputs = std::make_shared<ParseInputs>(Inputs) Feel free to keep as is too, I find the proposed style simpler to follow, but you could reasonably disagree. | |
unittests/clangd/ClangdTests.cpp | ||
1148 | Not strictly related to this patch, but maybe we could add a flag to completion results to indicate if the completion happened via a fallback mode or not? Would make the test code more straightforward and the tests like these would not rely on a particular implementation of the fallback mode (e.g. I can imagine the fallback mode learning about the scopes later on) | |
unittests/clangd/CodeCompleteTests.cpp | ||
1390 | Could you expand why we need this? |
LGTM! See the NITs, specifically about runAddDocument - those definitely look worth fixing.
unittests/clangd/ClangdTests.cpp | ||
---|---|---|
1113 | Ah, we should really have a wait-with-timeout for these use-cases. No need to do anything in this patch, though, that requires a change to Threading.h that is best done separately anyway. | |
1148 | Ah, Recovery looks good enough if we check the same location twice and second should be non-recovery. Maybe keep only the Context == Recovery check? Checking for particular results only seems to make test less focused. | |
unittests/clangd/CodeCompleteTests.cpp | ||
1390 | Ah, thanks! Could we instead use a sync helper here to keep the code a bit simpler (won't need a comment too)? runAddDocument(Server, ...); | |
1392 | Same here: could you use runAddDocument? |
NIT: this looks unrelated to the actual change. Maybe revert?