[SourceKit] ⚡️Fast code completion within function bodies #28727

rintaro · 2019-12-12T01:58:01Z

Reuse ASTContext(CompilerInstance) from the previous completions when possible.

ide::CompletionInstance manages CompilerInstance for completion like requests (i.e. code completion, type context info, and conforming method list).
It vends the cached CompilerInstance if:

This functionality is enabled, and
The compiler arguments (i.e. CompilerInvocation) has not changed, and
The primary file is the same, and
The completion happens inside function bodies in both previous and current completion, and
The interface hash of of the primary file has not changed

Since cached instance already have AST including imported modules, we just run the second pass.

Otherwise, CompletionInstance vends new CompilerInstance, so we run both first and second pass using that instance. This instance is cached for the next completion request.

NOTE: currently it's disabled by default.

For example, using this simple example:

func test(x: Int) {
  x.#^COMPLETE^#
}

# Disabled
$ sourcekitd-test -req=complete -pos=2:5 $PWD/test.swift -repeat-request=10 -dont-print-request -dont-print-response -time-request -- $PWD/test.swift
request time: 109.469 ms
request time: 102.269 ms
request time: 101.260 ms
request time: 99.622 ms
request time: 100.751 ms
request time: 100.154 ms
request time: 99.670 ms
request time: 100.965 ms
request time: 101.426 ms
request time: 100.784 ms

# Enabled
$ sourcekitd-test -req=complete -req-opts=reuseastcontext=1 -pos=2:5 $PWD/test.swift -repeat-request=10 -dont-print-request -dont-print-response -time-request -- $PWD/test.swift
request time: 108.051 ms
request time: 1.716 ms
request time: 1.553 ms
request time: 1.559 ms
request time: 1.466 ms
request time: 1.497 ms
request time: 1.445 ms
request time: 1.544 ms
request time: 1.447 ms
request time: 1.506 ms

rdar://problem/20787086

rintaro · 2019-12-12T01:58:12Z

@swift-ci Please test

swift-ci · 2019-12-12T03:23:38Z

Build failed
Swift Test Linux Platform
Git Sha - 205cfc40cb85a6ae33d1917182eb7cb99110d09d

rintaro · 2019-12-12T06:32:44Z

@swift-ci Please test Linux

rintaro · 2019-12-13T22:35:37Z

Forgot to add a test file.
@swift-ci Please test

swift-ci · 2019-12-13T22:38:04Z

Build failed
Swift Test Linux Platform
Git Sha - 205cfc40cb85a6ae33d1917182eb7cb99110d09d

swift-ci · 2019-12-13T22:45:22Z

Build failed
Swift Test OS X Platform
Git Sha - 205cfc40cb85a6ae33d1917182eb7cb99110d09d

tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h

include/swift/IDE/CompletionInstance.h

include/swift/Parse/LocalContext.h

lib/IDE/CompletionInstance.cpp

lib/Parse/ParseDecl.cpp

rintaro · 2019-12-17T21:55:37Z

@swift-ci Please test

swift-ci · 2019-12-17T21:55:58Z

Build failed
Swift Test Linux Platform
Git Sha - 4ccc35cbddc29a3b27cd8196e2a152dd5f5886ac

swift-ci · 2019-12-17T21:56:18Z

Build failed
Swift Test OS X Platform
Git Sha - 4ccc35cbddc29a3b27cd8196e2a152dd5f5886ac

rintaro · 2019-12-17T22:53:23Z

@swift-ci Please test Linux

benlangmuir

It would be good to have some additional tests:

Compiler argument changed
Source changed in a compatible way -> reuse
Source changed in an incompatible way -> new AST

You can modify the text that is passed without changing the compiler arguments by using -text-input option to sourcekitd-test I think.

rintaro · 2019-12-18T19:03:17Z

tools/SourceKit/tools/sourcekitd-test/sourcekitd-test.cpp

@@ -970,6 +972,8 @@ static int handleTestInvocation(TestOptions Opts, TestOptions &InitOpts) {
  if (Opts.SourceText) {
    sourcekitd_request_dictionary_set_string(Req, KeySourceText,
                                             Opts.SourceText->c_str());
+    sourcekitd_request_dictionary_set_string(Req, KeySourceFile,
+                                             SemaName.c_str());


This change was needed to use -name as the filename (key.sourcefile).

rintaro · 2019-12-18T19:03:38Z

@swift-ci Please smoke test

benlangmuir · 2019-12-18T19:09:21Z

tools/SourceKit/lib/SwiftLang/SwiftCompletion.cpp

-  Invocation.setCodeCompletionPoint(NewBuffer.get(), CodeCompletionOffset);
+  // Pin completion instance.
+  auto CompletionInst =  Lang.getCompletionInstance();
+  CompilerInstance *CI = CompletionInst->getCompilerInstance(


Oh hmm, this probably isn't thread safe. I don't think anything prevents you from making multiple code-completion requests (or other completion-like requests) at the same time. In sourcekitd-test, for example if you add -async and then make a bunch of requests in a row they probably overlap.

Perhaps we should use shared_ptr<CompilerInstance> so that if we renew it can't affect an ongoing completion?

Thanks! I confirmed the crashes with -async. Will fix.

rintaro · 2019-12-18T22:22:32Z

@benlangmuir The last commit changes the structure of the processing. So CompletionIntance take the control of the lifetime of CompilerInstace.
CompletionIntance::performOperation() receives a callback void(CompilerInstance &), and call it with the prepared compiler instance. This change has several benefits:

Prevent multiple completions use the same compiler instance at the same time.
Stacktraces are distinguishable between "fast" and "normal" mode
Consolidate more code between completion / type context / conforming method list

What do you think?

rintaro · 2019-12-18T22:22:40Z

@swift-ci Please smoke test

rintaro · 2019-12-18T22:49:25Z

Modified the way of storing the cached information. It should prevent CompilerInstance/ArgHash/ReuseCount from being out-of-sync.
@swift-ci Please smoke test

benlangmuir · 2019-12-19T01:40:37Z

Ah good point, a shared_ptr is wrong since you actually can't share this safely across threads. Note: your current code is still technically racy because the std::swap() of the state itself is not atomic.

So if you have racing completions, one will win and get to reuse the AST, and the next one will go through the slow path. Is that better than blocking on the first one? If you're reusing ASTs it seems like it would usually be better to wait since you have a chance to hit the fast path on every completion. Assuming the reuse case is 10x faster and that the number of concurrent completions is usually very small, you would be better off waiting.

If you're not reusing ASTs, then I guess running the completions in parallel might be better in some instances - or at least it matches the current behaviour.

rintaro · 2019-12-19T05:47:16Z

Fair enough. I update the PR to block other threads using std::mutex.
@swift-ci Please smoke test

Align with code completion.

Specifically, align with swiftTypeContextInfoImpl() and swiftConformingMethodListImpl()

So that we can use the different the buffer for the second pass from the first pass.

- Introduce ide::CompletionInstance to manage CompilerInstance - `CompletionInstance` vends the cached CompilerInstance when: -- The compiler arguments (i.e. CompilerInvocation) has has not changed -- The primary file is the same -- The completion happens inside function bodies in both previous and current completion -- The interface hash of the primary file has not changed - Otherwise, it vends a fresh CompilerInstance and cache it for the next completion rdar://problem/20787086

to getCachedCompilerInstance()

So we can pin it on the threads.

…ation Checking "selected" properties of CompilerInvocation is hard to keep it up to date. If the arguments are the same, the invocations are the same.

To controls the lifetime of CompilerInstance within CompletionIntance. - Prevent from using the same CompilerInstance from multiple completion requests - Separate the stacktrace between "fast" and "normal" completions - Further code consolidation between various completion-like requests

To prevent them from out-of-sync.

if fast-completion is enabled. So they have higher chance to use the cached completion instance. If it's disabled, don't block, use an ephemeral instance so we can peform multiple completions simultaneously.

benlangmuir · 2019-12-19T18:21:47Z

lib/IDE/CompletionInstance.cpp

-    CachedInst.swap(TheInstance);
+    CachedCI = std::move(TheInstance);
+    CachedArgHash = ArgsHash;
+    CachedReuseCound = 0;


Typo: "Cound" vs "Count"

benlangmuir · 2019-12-19T18:28:56Z

include/swift/IDE/CompletionInstance.h

+      llvm::function_ref<void(CompilerInstance &)> Callback);
+
+public:
+  void setEnableASTCaching(bool Flag) { EnableASTCaching = Flag; }


Even if this were done atomically it would not be correct for racing completions. Can we pass EnableASTCaching in as an argument to performOperation so that is is associated with a specific completion?

Done! How does it look now?

benlangmuir · 2019-12-19T18:30:29Z

lib/IDE/CompletionInstance.cpp

+
+  // If AST caching is enabled, block completions in other threads. So they
+  // have higher chance to use the cached completion instance.
+  Optional<std::lock_guard<std::mutex>> lock;


This is fun, but maybe too magical? Could we instead separate these code-paths, like

if (EnableASTCaching) { // Compute the signature of the invocation. llvm::hash_code ArgsHash(0); for (auto arg : Args) ArgsHash = llvm::hash_combine(ArgsHash, StringRef(arg)); // Concurrent completions will block so that they have a higher chance to use the cached completion instance. std::lock_guard<std::mutex> lock(mtx); if (performCachedOperaitonIfPossible(...)) return true; if (performNewOperation(...)) return true; } else { // Concurrent completions may happen in parallel when caching is disabled. if (performNewOperation(Invocation, ArgsHash, FileSystem, completionBuffer, Offset, Error, DiagC, Callback)) return true; }

so that it is associated with a specific completion.

rintaro · 2019-12-19T20:24:23Z

@swift-ci Please smoke test

rintaro requested review from akyrtzi and benlangmuir December 12, 2019 01:59

benlangmuir reviewed Dec 16, 2019

View reviewed changes

rintaro force-pushed the sourcekit-completion-reuseinstance branch 3 times, most recently from efbd9d6 to d36ae5d Compare December 17, 2019 21:54

rintaro requested a review from benlangmuir December 17, 2019 22:53

benlangmuir reviewed Dec 18, 2019

View reviewed changes

rintaro mentioned this pull request Dec 18, 2019

[SourceKit/swift-ide-test] Consolidate duplicated logics between completion like requests #28322

Closed

rintaro force-pushed the sourcekit-completion-reuseinstance branch from 0e2a41b to 6cd54e9 Compare December 18, 2019 19:02

rintaro commented Dec 18, 2019

View reviewed changes

benlangmuir reviewed Dec 18, 2019

View reviewed changes

rintaro force-pushed the sourcekit-completion-reuseinstance branch 2 times, most recently from f353894 to 5f7090a Compare December 18, 2019 22:14

rintaro requested a review from benlangmuir December 19, 2019 01:01

rintaro force-pushed the sourcekit-completion-reuseinstance branch from a4eee1a to 23f4416 Compare December 19, 2019 05:46

rintaro added 17 commits December 18, 2019 21:52

[SourceKit] Support VFS in 'typecontextinfo' and 'conformingmethods' req

f51f3b8

Align with code completion.

[CodeCompletion] Align swiftCodeCompleteImpl() with other similar funcs

cd284d1

Specifically, align with swiftTypeContextInfoImpl() and swiftConformingMethodListImpl()

[CodeCompletion] Use offsets in the buffer for second pass state

ff97c06

So that we can use the different the buffer for the second pass from the first pass.

[SourceKit] Add option to enable ASTContext reusing for code completion

c1530ee

[SourceKit] Calculate CompilerInvocation hash

a83c7e8

[SourceKit] Add a test case for fast code completion

fcc7e41

[SourceKit] Limit compiler instance reuse count for completions

c3d5828

[SourceKit] Add test file for fast completion

2160134

[CodeCompletion] Add doc-comment for CompletionInstance

204ae49

[CodeCompletion] Rename getReusingCompilerInstance()

f2466de

to getCachedCompilerInstance()

[SourceKit] Hold CompletionInstance with std::shared_ptr

c5b1ada

So we can pin it on the threads.

[CodeCompletion] Use the arguments to check the equality of the invoc…

0f45267

…ation Checking "selected" properties of CompilerInvocation is hard to keep it up to date. If the arguments are the same, the invocations are the same.

[SourceKit] Add more test cases for fast completions

fcb50d6

[CodeCompletion] Pack cached instance information into single struct

fe07d44

To prevent them from out-of-sync.

[CodeCompletionn] Block completions in other threads

2aec5d4

if fast-completion is enabled. So they have higher chance to use the cached completion instance. If it's disabled, don't block, use an ephemeral instance so we can peform multiple completions simultaneously.

benlangmuir reviewed Dec 19, 2019

View reviewed changes

rintaro force-pushed the sourcekit-completion-reuseinstance branch from 23f4416 to 4327111 Compare December 19, 2019 20:19

[SourceKit] Pass 'EnableASTCaching' flag as an argument

eebcbf6

so that it is associated with a specific completion.

rintaro force-pushed the sourcekit-completion-reuseinstance branch from 4327111 to eebcbf6 Compare December 19, 2019 20:20

benlangmuir approved these changes Dec 19, 2019

View reviewed changes

rintaro merged commit 10936f6 into swiftlang:master Dec 19, 2019

rintaro deleted the sourcekit-completion-reuseinstance branch December 19, 2019 22:26

rintaro mentioned this pull request Dec 19, 2019

[5.2][SourceKit] Fast code completion within function bodies #28884

Merged

dan-zheng mentioned this pull request Dec 22, 2019

Merge tag 'swift-DEVELOPMENT-SNAPSHOT-2019-12-20-a' into tensorflow #28924

Merged

[SourceKit] ⚡️Fast code completion within function bodies #28727

[SourceKit] ⚡️Fast code completion within function bodies #28727

Uh oh!

Conversation

rintaro commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Dec 12, 2019

Uh oh!

swift-ci commented Dec 12, 2019

Uh oh!

rintaro commented Dec 12, 2019

Uh oh!

rintaro commented Dec 13, 2019

Uh oh!

swift-ci commented Dec 13, 2019

Uh oh!

swift-ci commented Dec 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rintaro commented Dec 17, 2019

Uh oh!

swift-ci commented Dec 17, 2019

Uh oh!

swift-ci commented Dec 17, 2019

Uh oh!

rintaro commented Dec 17, 2019

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

rintaro Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

rintaro commented Dec 18, 2019

Uh oh!

benlangmuir Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

rintaro Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

rintaro commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Dec 18, 2019

Uh oh!

rintaro commented Dec 18, 2019

Uh oh!

benlangmuir commented Dec 19, 2019

Uh oh!

rintaro commented Dec 19, 2019

Uh oh!

benlangmuir Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

benlangmuir Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

rintaro Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

benlangmuir Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

rintaro commented Dec 19, 2019

Uh oh!

Uh oh!

rintaro commented Dec 12, 2019 •

edited

Loading

rintaro commented Dec 18, 2019 •

edited

Loading