[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<void(FuzzyFindRequest)> 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 &getRequest() const { return Req; }
+
----------------
Can this be a simple struct with two fields `std::future<SymbolSlab>` 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<FuzzyFindRequest> allRequests() const { return Requests; }
+  const std::vector<FuzzyFindRequest> 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