Page MenuHomePhabricator

[clangd] Wait for compile command in ASTWorker instead of ClangdServer
ClosedPublic

Authored by ioeric on Apr 12 2019, 3:56 AM.

Diff Detail

Event Timeline

ioeric created this revision.Apr 12 2019, 3:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 3:56 AM
ilya-biryukov added a comment.EditedApr 12 2019, 5:04 AM

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?
That might happen if a previous call to getCompileCommand succeeded (so the stored command does not have a heuristic set).

574

This is racy, FileInputs is only accessed from a worker thread now.
I'm afraid we'll need a separate variable with a lock around it (could reuse one lock for preamble and compile command, probably)

863

We should add a comment that compile command in inputs is ignored.
IMO even better is to accept an FS and Contents instead of ParseInputs.

ioeric updated this revision to Diff 194850.Apr 12 2019, 5:36 AM
ioeric marked 7 inline comments as done.
  • 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.

ioeric updated this revision to Diff 194851.Apr 12 2019, 5:40 AM
  • Add missing comment to TUScheduler.h
ilya-biryukov added inline comments.Apr 12 2019, 7:12 AM
clangd/TUScheduler.cpp
346

That makes sense. Could we remove the getFallbackCommand call from the ClangdServer::addDocument?
Let's make the TUScheduler fully responsible for the compile command.

372

Oh, I mixed Inputs and FileInputs in my head.
It feels reasonable to completely move the handling of compile command to TUScheduler, see the other comment at the constructor of ASTWorker.

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).
Doing the same thing we do for preamble looks like the best alternative here.

ioeric updated this revision to Diff 194891.Apr 12 2019, 8:10 AM
ioeric marked 5 inline comments as done.
  • 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.

ilya-biryukov added inline comments.Apr 12 2019, 9:58 AM
clangd/TUScheduler.cpp
574

Unfortunately we can't share Inputs.FS safely as the vfs implementations are not thread-safe.

ioeric marked an inline comment as done.Apr 12 2019, 10:24 AM
ioeric added inline comments.
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?

ilya-biryukov added inline comments.Apr 12 2019, 10:43 AM
clangd/TUScheduler.cpp
574

And FileInputs (incl. Inputs.FS) has always been available/shared across worker threads

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.
Even though the current patch does not add such racy accesses, it certainly makes it much easier to do it accidentally from the clients of ASTWorker in the future (one just needs to access getCurrentInputs().FS).

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.
Having a separate copy of the compile command is a small price to pay (both in terms of performance and complexity) to avoid exposing non-thread-safe members of ASTWorker in its public interface.

ioeric marked an inline comment as done.Apr 12 2019, 11:35 AM
ioeric added inline comments.
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.

ioeric marked an inline comment as done.Apr 12 2019, 11:36 AM
ioeric added inline comments.
clangd/TUScheduler.cpp
574

Inside ASTWorker, FileInputs can still be guarded with Mutex as a whole.

ioeric updated this revision to Diff 195111.Apr 15 2019, 2:38 AM
  • Only return compile command (instead of FileInputs) from AST worker.

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.
Maybe something like:

/// 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?
Thinking of something like:

// 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.
s/contains fallback command/does not have a command.

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?

ioeric updated this revision to Diff 195137.Apr 15 2019, 5:02 AM
ioeric marked 9 inline comments as done.
  • address comments
unittests/clangd/ClangdTests.cpp
1148

We are setting the context to Recovery and make fallback as part of Recovery. Do you we should distinguish fallback mode from Recovery?

unittests/clangd/CodeCompleteTests.cpp
1390

Added a comment.

ilya-biryukov accepted this revision.Apr 15 2019, 5:15 AM

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.
It's sad that we'll block indefinitely in the old implementation at this point. Having a failing test is much better than the one that never finishes.

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?

This revision is now accepted and ready to land.Apr 15 2019, 5:15 AM
ioeric updated this revision to Diff 195139.Apr 15 2019, 5:27 AM
ioeric marked 5 inline comments as done.
  • use sync runAddDocument in test
This revision was automatically updated to reflect the committed changes.