[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added inline comments.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:106-113
+    CI.setSema(nullptr);
+    CI.setASTConsumer(nullptr);
+    if (CI.getASTReader()) {
+      CI.getASTReader()->setDeserializationListener(nullptr);
+      /* This just sets consumer to null when DeserializationListener is null */
+      CI.getASTReader()->StartTranslationUnit(nullptr);
     }
----------------
kadircet wrote:
> why are we triggering destructors for all of these objects eagerly? if this is deliberate to "fix" some issue, could you mention that in comments?
> why are we triggering destructors for all of these objects eagerly? if this is deliberate to "fix" some issue, could you mention that in comments?

Thanks a lot for the review.
If we don't destruct and set it to null, CapturedASTCtx will also have to keep instances such as ASTConsumer including other related callbacks such PreambleCallbacks. This was making the CapturedASTCtx interface and implementation complex.


================
Comment at: clang-tools-extra/clangd/Preamble.cpp:694
     Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
-    return Result;
+    CapturedCtx.emplace(CapturedInfo.takeLife());
+    return std::make_pair(Result, CapturedCtx);
----------------
kadircet wrote:
> what about just keeping the callback (with a different signature) and calling it here? e.g.:
> ```
> PreambleCallback(CapturedInfo.takeLife());
> ```
> 
> that way we don't need to change the return type and also control if we received a valid object on every call site (since callback will only be invoked on success)
> what about just keeping the callback (with a different signature) and calling it here? e.g.:
> ```
> PreambleCallback(CapturedInfo.takeLife());
> ```
> 
> that way we don't need to change the return type and also control if we received a valid object on every call site (since callback will only be invoked on success)

Apologies for the misunderstanding.  Just to be clear, you prefer indexing in UpdateIndexCallbacks using the thread Tasks that also indexes in indexStdlib? In this implementation, I am calling the index action in preamble thread. I will revise it.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1063-1071
+      if (LatestBuild) {
+        assert(CapturedCtx);
+        CanonicalIncludes CanonIncludes = LatestBuild->CanonIncludes;
+        CompilerInvocation &CI = CapturedCtx->getCompilerInvocation();
+        ASTContext &Ctx = CapturedCtx->getASTContext();
+        Preprocessor &PP = CapturedCtx->getPreprocessor();
+        Callbacks.onPreambleAST(FileName, Inputs.Version, CI, Ctx, PP,
----------------
kadircet wrote:
> you can just execute this block around the call to `reportPreambleBuild` right? is there any particular reason to make this part of scope cleanup ? especially call it after the call to `updatePreamble` ?
> you can just execute this block around the call to `reportPreambleBuild` right? is there any particular reason to make this part of scope cleanup ? especially call it after the call to `updatePreamble` ?

Yes, we can do it around reportPreambleBuild. However, based on your previous comment, I got the impression that you prefer it be a callback to buildPreamble which indexes in the tasks. We could do it either way. 




================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1066-1068
+        CompilerInvocation &CI = CapturedCtx->getCompilerInvocation();
+        ASTContext &Ctx = CapturedCtx->getASTContext();
+        Preprocessor &PP = CapturedCtx->getPreprocessor();
----------------
kadircet wrote:
> these are all references to `CapturedCtx` which will be destroyed after the call. i guess we should be passing shared_ptrs/IntrusiveRefCntPtrs instead, right?
> 
> also I don't think it's enough to just pass these 3. we need to make sure rest of the fields in CapturedCtx is also kept alive (e.g. Preprocessor has raw pointers to Target/AuxTarget, which would be dangling after CapturedCtx goes out of scope).
> these are all references to `CapturedCtx` which will be destroyed after the call. i guess we should be passing shared_ptrs/IntrusiveRefCntPtrs instead, right?
> 
> also I don't think it's enough to just pass these 3. we need to make sure rest of the fields in CapturedCtx is also kept alive (e.g. Preprocessor has raw pointers to Target/AuxTarget, which would be dangling after CapturedCtx goes out of scope).

In this implementation, I am using these in the same scope. But I will revise it as per previous comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088