[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE340604: [clangd] Speculative code completion index request 
before Sema is run. (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50962?vs=162356=162358#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 #include "llvm/ADT/StringMap.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -167,6 +168,18 @@
   std::chrono::steady_clock::duration UpdateDebounce;
 };
 
+/// Runs \p Action asynchronously with a new std::thread. The context will be
+/// propogated.
+template 
+std::future runAsync(llvm::unique_function Action) {
+  return std::async(std::launch::async,
+[](llvm::unique_function &, Context Ctx) {
+  WithContext WithCtx(std::move(Ctx));
+  return Action();
+},
+std::move(Action), Context::current().clone());
+}
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -12,6 +12,7 @@
 #include "FindSymbols.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "Trace.h"
 #include "XRefs.h"
 #include "index/Merge.h"
 #include "clang/Format/Format.h"
@@ -22,13 +23,15 @@
 #include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
+#include 
 
 using namespace clang;
 using namespace clang::clangd;
@@ -181,21 +184,44 @@
   // Copy PCHs to avoid accessing this->PCHs concurrently
   std::shared_ptr PCHs = this->PCHs;
   auto FS = FSProvider.getFileSystem();
-  auto Task = [PCHs, Pos, FS,
-   CodeCompleteOpts](Path File, Callback CB,
- llvm::Expected IP) {
+
+  auto Task = [PCHs, Pos, FS, CodeCompleteOpts,
+   this](Path File, Callback CB,
+ llvm::Expected IP) {
 if (!IP)
   return CB(IP.takeError());
 
 auto PreambleData = IP->Preamble;
 
+llvm::Optional SpecFuzzyFind;
+if (CodeCompleteOpts.Index && CodeCompleteOpts.SpeculativeIndexRequest) {
+  SpecFuzzyFind.emplace();
+  {
+std::lock_guard Lock(CachedCompletionFuzzyFindRequestMutex);
+SpecFuzzyFind->CachedReq = CachedCompletionFuzzyFindRequestByFile[File];
+  }
+}
+
 // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
 // both the old and the new version in case only one of them matches.
 CodeCompleteResult Result = clangd::codeComplete(
 File, IP->Command, PreambleData ? >Preamble : nullptr,
 PreambleData ? PreambleData->Includes : IncludeStructure(),
-IP->Contents, Pos, FS, PCHs, CodeCompleteOpts);
-CB(std::move(Result));
+IP->Contents, Pos, FS, PCHs, CodeCompleteOpts,
+SpecFuzzyFind ? SpecFuzzyFind.getPointer() : 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162356.
ioeric added a comment.

- add doc


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 #include 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LG, thanks. And two small NITs.




Comment at: clangd/CodeComplete.h:184
+llvm::Expected
+speculateCompletionFilter(llvm::StringRef Content, Position Pos);
+

Is it exposed only for tests?
Maybe add a comment that it's a private API that should be avoided and put it 
to the end of the file?



Comment at: clangd/CodeComplete.h:201
+  /// The result is consumed by `codeComplete()` if speculation succeeded.
+  /// NOTE that the structure can only be destroyed after the async call
+  /// finishes.

A comment does not mention the destructor will wait for the async call to 
finish.
Maybe add that, it looks like an important detail?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162355.
ioeric added a comment.

- moved SpecReq into CodeComplete.cpp


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162351.
ioeric added a comment.

- fix doc


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 #include 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.h:187
+/// A speculative and asynchronous fuzzy find index request (based on cached
+/// request) that can be before parsing sema. This would reduce completion
+/// latency if the speculation succeeds.

ilya-biryukov wrote:
> s/can be before/can be send
> (or similar?)
or rather 'can be sent'


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/CodeComplete.h:187
+/// A speculative and asynchronous fuzzy find index request (based on cached
+/// request) that can be before parsing sema. This would reduce completion
+/// latency if the speculation succeeds.

s/can be before/can be send
(or similar?)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162348.
ioeric marked 5 inline comments as done.
ioeric added a comment.

- Merge remote-tracking branch 'origin/master' into speculate-index
- address comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests = {};
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:159
+  }
+  if (SpecFuzzyReq) {
+if (auto Filter = speculateCompletionFilter(Content, Pos)) {

ioeric wrote:
> ilya-biryukov wrote:
> > NIT: maybe invert condition to reduce nesting?
> It would become something like:
> ```
> if (!SpecFuzzyReq)
>   return SpecFuzzyReq;
>  // some work
> return SpecFuzzyReq;
> ```
> 
> Having to return the same thing twice seems a bit worse IMO.
I think it's better. Besides, you could replace return `llvm::None` in the 
first statement (would be even more straightforward from my point of view).
There's a secion in [[ 
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
 | LLVM Style Guide ]] on this.



Comment at: clangd/CodeComplete.cpp:1388
+  if (Len > 0)
+St++; // Shift to the first valid character.
+  return Content.substr(St, Len);

ioeric wrote:
> ilya-biryukov wrote:
> > This looks incorrect in case of the identifiers starting at offset 0. A 
> > corner case, but still.
> `Offset == 0` is handled above.
Didn't notice. LG, thanks



Comment at: clangd/CodeComplete.h:186
+
+struct SpeculativeFuzzyFind {
+  /// A cached request from past code completions.

Maybe a short comment describing on why we have this parameter?



Comment at: clangd/CodeComplete.h:196
+  /// Set by `codeComplete()`.
+  llvm::Optional NewReq;
+  /// Result from the speculative/asynchonous call. This is only valid if

`NewReq` is an implementation detail only needed in `CodeComplete.cpp`, right? 
Maybe remove from this struct and only set in .cpp file?



Comment at: clangd/CodeComplete.h:197
+  llvm::Optional NewReq;
+  /// Result from the speculative/asynchonous call. This is only valid if
+  /// `SpecReq` is set.

Maybe remove description of the value in the result?
We don't want anyone to use, so a simple comment that the future is only there 
to wait until the async calls complete should be enough.



Comment at: clangd/CodeComplete.h:217
+ std::shared_ptr PCHs,
+ SpeculativeFuzzyFind *SpecFuzzyFind, CodeCompleteOptions Opts);
 

SpecFuzzyFind is partially an out parameter, maybe put it at the end of the 
parameter list?



Comment at: clangd/tool/ClangdMain.cpp:179
+should be effective for a number of code completions.)"),
+llvm::cl::init(true));
+

ioeric wrote:
> ilya-biryukov wrote:
> > Maybe remove this option?
> > Doing a speculative request seems like the right thing to do in all cases. 
> > It never hurts completion latency. Any reasons to avoid it?
> Sure, was trying to be conservative :)
> 
> Removed the command-line option but still keeping the code completion option. 
> As async call has some overhead, I think we should only enable it when static 
> index is provided.
Enabling only when static index is there LG. OTOH, it should always take less 
than code completion in case there's no static index, so it would've probably 
been fine either way



Comment at: unittests/clangd/CodeCompleteTests.cpp:928
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;

Calling any methods on moved-from objects is a red flag.
It probably works on vectors, but maybe reassign `Requests = {}` or use a 
swap-with-empty-vector trick instead to avoid that?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162335.
ioeric added a comment.

- fix typos..


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 #include 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162224.
ioeric added a comment.

- another minior cleanup


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "Function.h"
 #include "Threading.h"
 #include 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/ClangdServer.cpp:159
+  }
+  if (SpecFuzzyReq) {
+if (auto Filter = speculateCompletionFilter(Content, Pos)) {

ilya-biryukov wrote:
> NIT: maybe invert condition to reduce nesting?
It would become something like:
```
if (!SpecFuzzyReq)
  return SpecFuzzyReq;
 // some work
return SpecFuzzyReq;
```

Having to return the same thing twice seems a bit worse IMO.



Comment at: clangd/CodeComplete.cpp:1388
+  if (Len > 0)
+St++; // Shift to the first valid character.
+  return Content.substr(St, Len);

ilya-biryukov wrote:
> This looks incorrect in case of the identifiers starting at offset 0. A 
> corner case, but still.
`Offset == 0` is handled above.



Comment at: clangd/tool/ClangdMain.cpp:179
+should be effective for a number of code completions.)"),
+llvm::cl::init(true));
+

ilya-biryukov wrote:
> Maybe remove this option?
> Doing a speculative request seems like the right thing to do in all cases. It 
> never hurts completion latency. Any reasons to avoid it?
Sure, was trying to be conservative :)

Removed the command-line option but still keeping the code completion option. 
As async call has some overhead, I think we should only enable it when static 
index is provided.



Comment at: unittests/clangd/CodeCompleteTests.cpp:926
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);

ilya-biryukov wrote:
> Why do we want to change this? To avoid tests getting requests from the 
> previous tests?
Yes. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 16.
ioeric added a comment.

- minor cleanup


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -20,6 +20,7 @@
 #include "llvm/Support/StringSaver.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +344,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -8,6 +8,9 @@
 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 162221.
ioeric marked 5 inline comments as done.
ioeric added a comment.
Herald added subscribers: jfb, javed.absar.

- Moved most logic into CodeComplete.cc


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/TUScheduler.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1705,6 +1709,75 @@
 Not(Contains(Labeled("void vfunc(bool param) override");
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -308,6 +308,7 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -19,7 +19,9 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/StringSaver.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -343,6 +345,14 @@
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector ProximityPaths;
+
+  bool operator==(const FuzzyFindRequest ) const {
+return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,
+ProximityPaths) ==
+   std::tie(Req.Query, Req.Scopes, Req.MaxCandidateCount,
+Req.RestrictForCodeCompletion, Req.ProximityPaths);
+  }
+  bool operator!=(const FuzzyFindRequest ) const { return !(*this == Req); }
 };
 
 struct LookupRequest {
Index: clangd/index/Index.cpp

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Really excited about this one, this should give us decent latency improvements 
when using the static index.
My main suggestion would be to put almost all of the speculating code into 
`CodeComplete.cpp`.

We could merely return the request that should be used for speculation to 
`ClangdServer`, that would in turn stash it in the map.
This should keep the code more localized in code completion and keep changes to 
other parts trivial. WDYT?




Comment at: clangd/ClangdServer.cpp:159
+  }
+  if (SpecFuzzyReq) {
+if (auto Filter = speculateCompletionFilter(Content, Pos)) {

NIT: maybe invert condition to reduce nesting?



Comment at: clangd/ClangdServer.cpp:160
+  if (SpecFuzzyReq) {
+if (auto Filter = speculateCompletionFilter(Content, Pos)) {
+  SpecFuzzyReq->Query = *Filter;

Maybe move this logic into code completion, keeping only the storage for the 
old requests?
The code is domain-specific to code completion and we should aim to keep as 
little of domain logic in `ClangdServer`.



Comment at: clangd/CodeComplete.cpp:1053
   IncludeStructure Includes; // Complete once the compiler runs.
+  std::function UpdateCachedFuzzyFindReq;
+  AsyncFuzzyFind *SpeculativeFuzzyFind; // Can be nullptr.

Maybe return the new callback from completion instead of passing a callback to 
update here?



Comment at: clangd/CodeComplete.cpp:1388
+  if (Len > 0)
+St++; // Shift to the first valid character.
+  return Content.substr(St, Len);

This looks incorrect in case of the identifiers starting at offset 0. A corner 
case, but still.



Comment at: clangd/index/Index.cpp:147
+  Result =
+  std::async(std::launch::async, QueryIndex, Context::current().clone());
+}

I think we should just have a generic 'std::async'-like helper that does the 
same things, properly piping our context to the new threads.
Async requests to the index and other code potentially doing network requests 
are good candidates to use use it.

Maybe include it in this change? It shouldn't be more than a few lines, but 
would already make the code cleaner.



Comment at: clangd/index/Index.h:404
+/// any, finishes.
+class AsyncFuzzyFind {
+public:

Could this be an implementation detail of code completion if we move the 
spawning of the task into `codeComplete` function?



Comment at: clangd/index/Index.h:409
+
+  const FuzzyFindRequest () const { return Req; }
+

Can this be a simple struct with two fields `std::future` and the 
original `FuzzyFindRequest`?
And having a function that fires the async request instead of doing that in 
constructor should be more straight-forward



Comment at: clangd/tool/ClangdMain.cpp:179
+should be effective for a number of code completions.)"),
+llvm::cl::init(true));
+

Maybe remove this option?
Doing a speculative request seems like the right thing to do in all cases. It 
never hurts completion latency. Any reasons to avoid it?



Comment at: unittests/clangd/CodeCompleteTests.cpp:926
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);

Why do we want to change this? To avoid tests getting requests from the 
previous tests?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 161937.
ioeric added a comment.

- minor cleanup.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/IndexTests.cpp

Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -321,6 +321,21 @@
   EXPECT_EQ(M.Name, "right");
 }
 
+TEST(AsyncFuzzyFind, Simple) {
+  MemIndex Idx;
+  Idx.build(generateSymbols({"ns::abc", "ns::xyz"}));
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  Req.Scopes = {"ns::"};
+
+  AsyncFuzzyFind Async(Idx, Req);
+
+  EXPECT_EQ(Async.getRequest(), Req);
+  EXPECT_THAT(Async.getResult(),
+  UnorderedElementsAre(Named("abc"), Named("xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1700,6 +1704,75 @@
   }
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative request is the same as the one generated
+for the ongoing 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: unittests/clangd/CodeCompleteTests.cpp:1710
+  $bol^
+  ab$ab^
+  x.ab$dot^

kadircet wrote:
> Maybe an EXPECT for this one as well?
Nice catch!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 161651.
ioeric marked an inline comment as done.
ioeric added a comment.

- Improve tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/IndexTests.cpp

Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -321,6 +321,21 @@
   EXPECT_EQ(M.Name, "right");
 }
 
+TEST(AsyncFuzzyFind, Simple) {
+  MemIndex Idx;
+  Idx.build(generateSymbols({"ns::abc", "ns::xyz"}));
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  Req.Scopes = {"ns::"};
+
+  AsyncFuzzyFind Async(Idx, Req);
+
+  EXPECT_EQ(Async.getRequest(), Req);
+  EXPECT_THAT(Async.getResult(),
+  UnorderedElementsAre(Named("abc"), Named("xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1700,6 +1704,75 @@
   }
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("ab"), "ab");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  auto CompleteAtPoint = [&](StringRef P) {
+cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
+// Sleep for a while to make sure asynchronous call (if applicable) is also
+// triggered before callback is invoked.
+std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  };
+
+  CompleteAtPoint("1");
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  CompleteAtPoint("2");
+  auto Reqs2 = Requests.consumeRequests();
+  // Speculation succeeded. Used speculative index result.
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  CompleteAtPoint("3");
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative request is the same as 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: unittests/clangd/CodeCompleteTests.cpp:1710
+  $bol^
+  ab$ab^
+  x.ab$dot^

Maybe an EXPECT for this one as well?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 161537.
ioeric added a comment.

- Make sure completion result callback can be called even if the unused 
speculative request has not finished.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/IndexTests.cpp

Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -321,6 +321,21 @@
   EXPECT_EQ(M.Name, "right");
 }
 
+TEST(AsyncFuzzyFind, Simple) {
+  MemIndex Idx;
+  Idx.build(generateSymbols({"ns::abc", "ns::xyz"}));
+
+  FuzzyFindRequest Req;
+  Req.Query = "";
+  Req.Scopes = {"ns::"};
+
+  AsyncFuzzyFind Async(Idx, Req);
+
+  EXPECT_EQ(Async.getRequest(), Req);
+  EXPECT_THAT(Async.getResult(),
+  UnorderedElementsAre(Named("abc"), Named("xyz")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -923,7 +923,11 @@
llvm::function_ref
Callback) const override {}
 
-  const std::vector allRequests() const { return Requests; }
+  const std::vector consumeRequests() const {
+auto Reqs = std::move(Requests);
+Requests.clear();
+return Reqs;
+  }
 
 private:
   mutable std::vector Requests;
@@ -934,7 +938,7 @@
   IndexRequestCollector Requests;
   Opts.Index = 
   completions(Code, {}, Opts);
-  return Requests.allRequests();
+  return Requests.consumeRequests();
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
@@ -1700,6 +1704,71 @@
   }
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  namespace ns1 { int abc; }
+  namespace ns2 { int abc; }
+  void f() { ns1::ab$1^; ns1::ab$2^; }
+  void f() { ns2::ab$3^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  IndexRequestCollector Requests;
+  Opts.Index = 
+  Opts.SpeculativeIndexRequest = true;
+
+  cantFail(runCodeComplete(Server, File, Test.point("1"), Opts));
+  auto Reqs1 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs1.size(), 1u);
+  EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
+
+  // Speculation succeeded. Use speculative index result.
+  cantFail(runCodeComplete(Server, File, Test.point("2"), Opts));
+  auto Reqs2 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs2.size(), 1u);
+  EXPECT_EQ(Reqs2[0], Reqs1[0]);
+
+  cantFail(runCodeComplete(Server, File, Test.point("3"), Opts));
+  // Sleep for a while to make sure asynchronous call is also triggered before
+  // callback is invoked.
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+
+  // Speculation failed. Sent speculative index request and the new index
+  // request after sema.
+  auto Reqs3 = Requests.consumeRequests();
+  ASSERT_EQ(Reqs3.size(), 2u);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative request is the same as 

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric planned changes to this revision.
ioeric added a comment.

I just realized that `CodeCompleteFlow` would be blocked by the speculative 
index request even when index request is not needed (e.g. member access), 
because it owns the future object. This can make some completion requests 
slower. We may need to move the async index query into the 
`ClangdServer::codeComplete` so that completion callback can be invoked as soon 
as results are ready, without having to wait for speculative index results.

F7000668: future-redundant-wait.png 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962



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

[PATCH] D50962: [clangd] Speculative code completion index request before Sema is run.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.

For index-based code completion, send an asynchronous speculative index
request, based on the index request for the last code completion on the same
file and the filter text typed before the cursor, before sema code completion
is invoked. This can reduce the code completion latency (by roughly latency of
sema code completion) if the speculative request is the same as the one
generated for the ongoing code completion from sema. As a sequence of code
completions often have the same scopes and proximity paths etc, this should be
effective for a number of code completions.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50962

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1594,6 +1594,66 @@
   ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
 }
 
+TEST(SpeculateCompletionFilter, Filters) {
+  Annotations F(R"cpp($bof^
+  $bol^
+  ab$ab^
+  x.ab$dot^
+  x.$dotempty^
+  x::ab$scoped^
+  x::$scopedempty^
+
+  )cpp");
+  auto speculate = [&](StringRef PointName) {
+auto Filter = speculateCompletionFilter(F.code(), F.point(PointName));
+assert(Filter);
+return *Filter;
+  };
+  EXPECT_EQ(speculate("bof"), "");
+  EXPECT_EQ(speculate("bol"), "");
+  EXPECT_EQ(speculate("dot"), "ab");
+  EXPECT_EQ(speculate("dotempty"), "");
+  EXPECT_EQ(speculate("scoped"), "ab");
+  EXPECT_EQ(speculate("scopedempty"), "");
+}
+
+TEST(CompletionTest, EnableSpeculativeIndexRequest) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  FS.Files[testPath("bar.h")] =
+  R"cpp(namespace ns { struct preamble { int member; }; })cpp";
+  auto File = testPath("foo.cpp");
+  Annotations Test(R"cpp(
+  #include "bar.h"
+  namespace ns1 { int local1; }
+  namespace ns2 { int local2; }
+  void f() { ns1::^; }
+  void f() { ns2::$2^; }
+  )cpp");
+  runAddDocument(Server, File, Test.code());
+  clangd::CodeCompleteOptions Opts = {};
+
+  auto I = memIndex({var("ns1::index1"), var("ns2::index2")});
+  Opts.Index = I.get();
+  Opts.SpeculativeIndexRequest = true;
+
+  auto First = cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  EXPECT_THAT(First.Completions,
+  UnorderedElementsAre(Named("local1"), Named("index1")));
+
+  auto Second = cantFail(runCodeComplete(Server, File, Test.point(), Opts));
+  EXPECT_THAT(Second.Completions,
+  UnorderedElementsAre(Named("local1"), Named("index1")));
+
+  auto CacheMiss =
+  cantFail(runCodeComplete(Server, File, Test.point("2"), Opts));
+  EXPECT_THAT(CacheMiss.Completions,
+  UnorderedElementsAre(Named("local2"), Named("index2")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -164,6 +164,20 @@
"an include line will be inserted or not."),
 llvm::cl::init(true));
 
+static llvm::cl::opt SpeculateCompletionIndexRequest(
+"speculative-completion-index-request",
+llvm::cl::desc(
+R"(If set to true and static index is enabled, this will send an
+asynchronous speculative index request, based on the index request for
+the last code completion on the same file and the filter text typed
+before the cursor, before sema code completion is invoked. This can
+reduce the code completion latency (by roughly latency of sema code
+completion) if the speculative request is the same as the one generated
+for the ongoing code completion from sema. As a sequence of code
+completions often have the same scopes and proximity paths etc, this
+should be effective for a number of code completions.)"),
+llvm::cl::init(true));
+
 static llvm::cl::opt YamlSymbolFile(
 "yaml-symbol-file",
 llvm::cl::desc(
@@ -299,6 +313,8 @@
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
+  CCOpts.SpeculativeIndexRequest =
+  SpeculateCompletionIndexRequest && Opts.StaticIndex;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -18,7 +18,10 @@