[PATCH] D155619: [clangd] Always run preamble indexing on a separate thread

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet closed this revision.
kadircet added a comment.

landed with 27ade4b554774187d2c0afcf64cd16fa6d5f619d 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155619

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155619: [clangd] Always run preamble indexing on a separate thread

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 541938.
kadircet added a comment.

Fix some tsan issues


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155619

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Preamble.cpp

Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -11,6 +11,8 @@
 #include "Compiler.h"
 #include "Config.h"
 #include "Diagnostics.h"
+#include "FS.h"
+#include "FeatureModule.h"
 #include "Headers.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -21,8 +23,10 @@
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticLex.h"
+#include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
@@ -51,12 +55,17 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
+#include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -616,7 +625,8 @@
   });
   llvm::IntrusiveRefCntPtr PreambleDiagsEngine =
   CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
-  &PreambleDiagnostics, false);
+  &PreambleDiagnostics,
+  /*ShouldOwnClient=*/false);
   const Config &Cfg = Config::current();
   PreambleDiagnostics.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
@@ -663,8 +673,12 @@
   auto BuiltPreamble = PrecompiledPreamble::Build(
   CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine,
   Stats ? TimedFS : StatCacheFS, std::make_shared(),
-  StoreInMemory, /*StoragePath=*/StringRef(), CapturedInfo);
+  StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+  // Reset references to ref-counted-ptrs before executing the callbacks, to
+  // prevent resetting them concurrently.
+  PreambleDiagsEngine.reset();
+  CI.DiagnosticOpts.reset();
 
   // When building the AST for the main file, we do want the function
   // bodies.
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -185,10 +185,6 @@
 /// regions in the document.
 bool PublishInactiveRegions = false;
 
-/// Whether to run preamble indexing asynchronously in an independent
-/// thread.
-bool AsyncPreambleIndexing = false;
-
 explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -67,11 +67,10 @@
   UpdateIndexCallbacks(FileIndex *FIndex,
ClangdServer::Callbacks *ServerCallbacks,
const ThreadsafeFS &TFS, AsyncTaskRunner *Tasks,
-   bool CollectInactiveRegions,
-   const ClangdServer::Options &Opts)
-  : FIndex(FIndex), ServerCallbacks(ServerCallbacks),
-TFS(TFS), Stdlib{std::make_shared()}, Tasks(Tasks),
-CollectInactiveRegions(CollectInactiveRegions), Opts(Opts) {}
+   bool CollectInactiveRegions)
+  : FIndex(FIndex), ServerCallbacks(ServerCallbacks), TFS(TFS),
+Stdlib{std::make_shared()}, Tasks(Tasks),
+CollectInactiveRegions(CollectInactiveRegions) {}
 
   void onPreambleAST(
   PathRef Path, llvm::StringRef Version, CapturedASTCtx ASTCtx,
@@ -94,7 +93,7 @@
  ASTCtx.getPreprocessor(), *CanonIncludes);
 };
 
-if (Opts.AsyncPreambleIndexing && Tasks) {
+if (Tasks) {
   Tasks->runAsync("Preamble indexing for:" + Path + Version,
   std::move(Task));
 } else
@@ -164,7 +163,6 @@
   std::shared_ptr Stdlib;
   AsyncTaskRunner *Tasks;
   bool CollectInactiveRegions;
-  const ClangdServer::Options &Opts;
 };
 
 class DraftStoreFS : public ThreadsafeFS {
@@ -229,7 +227,7 @@
 std::make_unique(
 DynamicIdx.get(), Callbacks, TFS,
 

[PATCH] D155619: [clangd] Always run preamble indexing on a separate thread

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG036a1b2202cb: [clangd] Always run preamble indexing on a 
separate thread (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155619

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h


Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -185,10 +185,6 @@
 /// regions in the document.
 bool PublishInactiveRegions = false;
 
-/// Whether to run preamble indexing asynchronously in an independent
-/// thread.
-bool AsyncPreambleIndexing = false;
-
 explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -94,7 +94,7 @@
  ASTCtx.getPreprocessor(), *CanonIncludes);
 };
 
-if (Opts.AsyncPreambleIndexing && Tasks) {
+if (Tasks) {
   Tasks->runAsync("Preamble indexing for:" + Path + Version,
   std::move(Task));
 } else


Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -185,10 +185,6 @@
 /// regions in the document.
 bool PublishInactiveRegions = false;
 
-/// Whether to run preamble indexing asynchronously in an independent
-/// thread.
-bool AsyncPreambleIndexing = false;
-
 explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -94,7 +94,7 @@
  ASTCtx.getPreprocessor(), *CanonIncludes);
 };
 
-if (Opts.AsyncPreambleIndexing && Tasks) {
+if (Tasks) {
   Tasks->runAsync("Preamble indexing for:" + Path + Version,
   std::move(Task));
 } else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155619: [clangd] Always run preamble indexing on a separate thread

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This has been the default in our production setup for weeks now,
showing great improvements to latency and no problems around stability or
correctness of the results.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155619

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h


Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -185,10 +185,6 @@
 /// regions in the document.
 bool PublishInactiveRegions = false;
 
-/// Whether to run preamble indexing asynchronously in an independent
-/// thread.
-bool AsyncPreambleIndexing = false;
-
 explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -94,7 +94,7 @@
  ASTCtx.getPreprocessor(), *CanonIncludes);
 };
 
-if (Opts.AsyncPreambleIndexing && Tasks) {
+if (Tasks) {
   Tasks->runAsync("Preamble indexing for:" + Path + Version,
   std::move(Task));
 } else


Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -185,10 +185,6 @@
 /// regions in the document.
 bool PublishInactiveRegions = false;
 
-/// Whether to run preamble indexing asynchronously in an independent
-/// thread.
-bool AsyncPreambleIndexing = false;
-
 explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -94,7 +94,7 @@
  ASTCtx.getPreprocessor(), *CanonIncludes);
 };
 
-if (Opts.AsyncPreambleIndexing && Tasks) {
+if (Tasks) {
   Tasks->runAsync("Preamble indexing for:" + Path + Version,
   std::move(Task));
 } else
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits