This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Speculative code completion index request before Sema is run.
ClosedPublic

Authored by ioeric on Aug 20 2018, 2:53 AM.

Details

Summary

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.

Trace with speculative index request:

Event Timeline

ioeric created this revision.Aug 20 2018, 2:53 AM
ioeric edited the summary of this revision. (Show Details)Aug 20 2018, 2:55 AM
ioeric planned changes to this revision.Aug 20 2018, 8:26 AM

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.

ioeric updated this revision to Diff 161537.Aug 20 2018, 12:27 PM
  • Make sure completion result callback can be called even if the unused speculative request has not finished.
kadircet added inline comments.Aug 20 2018, 2:24 PM
unittests/clangd/CodeCompleteTests.cpp
1690

Maybe an EXPECT for this one as well?

ioeric updated this revision to Diff 161651.Aug 21 2018, 12:36 AM
ioeric marked an inline comment as done.
  • Improve tests.
unittests/clangd/CodeCompleteTests.cpp
1690

Nice catch!

ioeric updated this revision to Diff 161937.Aug 22 2018, 6:05 AM
  • minor cleanup.

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?

clangd/ClangdServer.cpp
188

NIT: maybe invert condition to reduce nesting?

189

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.

clangd/CodeComplete.cpp
1163

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

1519

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

clangd/index/Index.cpp
146

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.

clangd/index/Index.h
402

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

407

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

clangd/tool/ClangdMain.cpp
188

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?

unittests/clangd/CodeCompleteTests.cpp
926–930

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

ioeric updated this revision to Diff 162221.Aug 23 2018, 10:22 AM
ioeric marked 5 inline comments as done.
  • Moved most logic into CodeComplete.cc
ioeric updated this revision to Diff 162222.Aug 23 2018, 10:24 AM
  • minor cleanup
ioeric updated this revision to Diff 162224.Aug 23 2018, 10:25 AM
  • another minior cleanup
ioeric added inline comments.Aug 23 2018, 10:25 AM
clangd/ClangdServer.cpp
188

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.

clangd/CodeComplete.cpp
1519

Offset == 0 is handled above.

clangd/tool/ClangdMain.cpp
188

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.

unittests/clangd/CodeCompleteTests.cpp
926–930

Yes.

ioeric updated this revision to Diff 162335.Aug 24 2018, 1:17 AM
  • fix typos..
ilya-biryukov added inline comments.Aug 24 2018, 2:03 AM
clangd/ClangdServer.cpp
188

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 LLVM Style Guide on this.

clangd/CodeComplete.cpp
1519

Didn't notice. LG, thanks

clangd/CodeComplete.h
185

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

195

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

196

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.

216

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

clangd/tool/ClangdMain.cpp
188

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

unittests/clangd/CodeCompleteTests.cpp
928

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?

ioeric updated this revision to Diff 162348.Aug 24 2018, 2:15 AM
ioeric marked 5 inline comments as done.
  • Merge remote-tracking branch 'origin/master' into speculate-index
  • address comments.
ilya-biryukov added inline comments.Aug 24 2018, 2:20 AM
clangd/CodeComplete.h
186

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

ilya-biryukov added inline comments.Aug 24 2018, 2:22 AM
clangd/CodeComplete.h
186

or rather 'can be sent'

ioeric updated this revision to Diff 162351.Aug 24 2018, 2:25 AM
  • fix doc
ioeric updated this revision to Diff 162355.Aug 24 2018, 3:01 AM
  • moved SpecReq into CodeComplete.cpp
ilya-biryukov accepted this revision.Aug 24 2018, 3:02 AM

LG, thanks. And two small NITs.

clangd/CodeComplete.h
183

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?

200

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

This revision is now accepted and ready to land.Aug 24 2018, 3:02 AM
ioeric updated this revision to Diff 162356.Aug 24 2018, 3:08 AM
  • add doc
This revision was automatically updated to reflect the committed changes.