ping? Any thoughts. Thanks.

Updated based on the review comments.

Set just PrecompilePreambleConsumer/PCHGenerator and ASTMutationListener to null.
Set the FileManager VFS to consuming FS so that it is thread safe.

Revised Except for:

1. Explicitly destructing in AfterExecute
2. setStatCache to take refence

Removing Both of which is causing crash.

Fixed missing merge that caused build error.

As per review, moved Preamble indexing into ClangdServer's IndexTasks.

Fixed tests
clangd/unittests/TUSchedulerTests.cpp require re-warite due to delayed onPreambleAST callback.

Re-implemented based on review

In D148088#4316352, @kadircet wrote:

Sorry I was trying to give some brief idea about what it might look like in Implementation Concerns section above, but let me give some more details;

I think we can just change the signature for PreambleParsedCallback to pass along refcounted objects. forgot to mention in the first comment, but we should also change the CanonicalIncludes to be a shared_ptr so that it can outlive the PreambleData. We should invoke the callback inside buildPreamble after a successful build. Afterwards we should also change the signature for onPreambleAST to take AST, PP and CanonicalIncludes as ref-counted objects again and PreambleThread::build should just forward objects received from PreambleParsedCallback. Afterwards inside the UpdateIndexCallbacks::onPreambleAST we can just invoke indexing on Tasks if it's present or synchronously in the absence of it.

Thanks @kadircet for the details.

Thanks @kadircet , @sammccall and @ilya-biryukov for the super detailed explanation. As mentioned, we will test clangd with an ASAN build to test.

In D148088#4302092, @ilya-biryukov wrote:

We would like to move the preamble index out of the critical path

Could you provide motivation why you need to do it? What is the underlying problem that this change is trying to solve?
We rely on preamble being indexed at that particular time for correct results in future steps, it's definitely not a no-op change even if the threading issues are resolved (see the other comment).

if you don't mind me asking, are you deliberately building remote-index components? i.e. do you have an internal remote-index server/deployment ? Asking as it'd be great to know that we've adoption here, outside of ourselves.

OTOH, if you're not actually using it, this shouldn't be built unless -DCLANGD_ENABLE_REMOTE=1 is part of the build config, hence we might have a bug in the build configuration and I'd like to make sure it's fixed.

Thanks for looking into it. Yes, we use -DCLANGD_ENABLE_REMOTE=On and provide grpc path with -DGRPC_INSTALL_PATH.

In D141950#4141455, @aaron.ballman wrote:

LGTM! You should probably put "NFC" into the patch title when landing the changes though, so it's more clear as to why there's no test coverage.

Do you think this warrants adding a release note to let users know about the performance improvement?

Rebase

Rebasing branch.

@sammccall, What are the first steps that is needed for supporting modules with clangd. From what I see, options related to handling comments that are specifically added to the compile options by clangd needs handling. This RFC diff is on that note.

Thanks a lot for the detailed comment. Yes, you are right in that we are using build system (buck) to build the modules and provide as part of the CDB. This is fairly cheap when we hit the buck cache that is also shared by others.