[Code Completion] Split code completion results into context free part that can be cached and contextual part displayed to the user #40471

ahoppen · 2021-12-08T13:49:47Z

This is the first (of two) refactoring part of #40399, which has gotten a little big.

This allows makes the distinction between cachable and non-cachable properties cleaner and allows us to more easily compute contextual information (like type relations) for cached items later.

ahoppen · 2021-12-08T13:50:47Z

@swift-ci Please smoke test

rintaro · 2021-12-10T03:56:17Z

include/swift/IDE/CodeCompletion.h

+  /// Maps USRs to \c CodeCompletionResultTypes that have already been
+  /// constructed.
+  static llvm::StringMap<CodeCompletionResultType *> USRCache;
+
+  /// Maps Types to \c CodeCompletionResultTypes that have already been
+  /// constructed.
+  static llvm::DenseMap<Type, CodeCompletionResultType *> TypeCache;


Global cache without clearing. This is a little scary, and they leaks. I believe these are tied to an ASTContext. Doesn't it make sense to move this to ASTContext instance member?

Also, can CodeCompletionResultType be allocated on the ASTContext?

Oh, I thought that cache would only be introduced by the next PR, sorry.

I will store them in a dedicate arena with ahoppen@e9ef698 but because that change is >200 LOC as well, I didn’t want to inflate this PR even further. Do you think we should pull that commit into this PR or should we just fix it in a follow-up PR?

We can’t store CodeCompletionResultType in the ASTContext because they are used for cached results, which are used for multiple code completion sessions and thus outlive the ASTContext.

We can’t store CodeCompletionResultType in the ASTContext because they are used for cached results, which are used for multiple code completion sessions and thus outlive the ASTContext.

I'm not sure the cache should outlive the ASTContext. An ASTContext is used for multiple code completion sessions as long as fast-completion is working. If fast-completion is not working and the ASTContext is destroyed, I think the cache should be cleared, or what cache can we reuse?

ahoppen · 2021-12-10T08:34:04Z

@swift-ci Please smoke test Windows

ahoppen · 2021-12-13T13:48:07Z

@swift-ci Please test Windows

ahoppen · 2021-12-14T20:01:37Z

I refactored the PR to make a cleaner distinction between USR-based and ASTContext-based types. For now ContextFreeCodeCompletionResult stores a CodeCompletionResultType that is always backed by a ASTContext-bound swift::Type. That design doesn’t allow caching the types yet.

In a follow-up PR I will add the option to back CodeCompletionResultType by a swift::ide::USRBasedType which can be cached and supports computation of convertible type relations by storing the USRs of its supertype. The current WIP of that PR lives at https://github.com/ahoppen/swift/tree/pr/compute-typerelation-for-global-cached-results.

ahoppen · 2021-12-14T20:01:43Z

@swift-ci Please smoke test

rintaro · 2021-12-20T23:10:20Z

include/swift/IDE/CodeCompletion.h

+/// A single code completion result enriched with information that depend on
+/// the completion's usage context.
+class CodeCompletionResult {
+  ContextFreeCodeCompletionResult ContextFree;


ContextFreeCodeCompletionResult is fairly big object. Instead of copying it, could CodeCompletionResult just hold a reference to it?

I thought about the same but decided against it, because

this way, we are sure that ContextFreeCodeCompletionResult is allocated by the same allocator as CodeCompletionResult and we don’t run into issues that ContextFreeCodeCompletionResult might get deallocated while CodeCompletionResult is still referencing it

we only need to copy it when constructing CodeCompletionResult and I don’t think it makes much of a performance difference

We don’t have to do an indirection to ContextFreeCodeCompletionResult when accessing context-free properties of a CodeCompletionResult

Do you think this makes sense or would you think it’s worth to measure the performance impact of the copy?

ahoppen · 2022-01-14T16:29:18Z

I have rebased this PR now that #40724 is merged and tried to make it as easy to review as possible:

707e6e855bfc3a6cc65c0d71b408bf63ee9b56ab is a super simple refactoring that just pull out a few nested types from CodeCompletionResult to the top-level
e0253ca624e2709c3e4c5a2f88344251c0b72888 performs the actual split into contextual and context-free result bits.

ahoppen · 2022-01-14T16:29:25Z

@swift-ci Please smoke test

ahoppen · 2022-01-19T15:29:05Z

@swift-ci Please smoke test Windows

ahoppen · 2022-01-20T09:37:59Z

@swift-ci Please smoke test Windows

…ultKind to top-level types Preliminary work to split `CodeCompletionResult` into a context-free and contextual part.

ahoppen · 2022-01-20T17:00:59Z

@swift-ci Please smoke test

rintaro

LGTM!

rintaro · 2022-01-27T04:21:36Z

include/swift/IDE/CodeCompletion.h

+  CodeCompletionResultKind Kind : 3;
+  static_assert(int(CodeCompletionResultKind::MAX_VALUE) < 1 << 3, "");
+
+  /// Opaque kind that is interpreted differently depending on


Nit: Incomplete comment? Probably:

Suggested change

/// Opaque kind that is interpreted differently depending on

/// Opaque kind that is interpreted differently depending on 'Kind'.

Thanks. That comment pre-dated the use of a union here. I just removed it.

…t that can be cached and contextual part displayed to the user This allows makes the distinction between cachable and non-cachable properties cleaner and allows us to more easily compute contextual information (like type relations) for cached items later.

ahoppen · 2022-01-27T09:41:24Z

@swift-ci Please smoke test

ahoppen requested a review from rintaro December 8, 2021 13:49

ahoppen mentioned this pull request Dec 8, 2021

[SourceKit] Compute type relations for global code completion items from the cache #40399

Closed

rintaro reviewed Dec 10, 2021

View reviewed changes

ahoppen force-pushed the pr/split-context-free-codecompletion-result branch from a704547 to 23f0d6e Compare December 14, 2021 19:56

rintaro reviewed Dec 20, 2021

View reviewed changes

ahoppen force-pushed the pr/split-context-free-codecompletion-result branch from 23f0d6e to e0253ca Compare January 14, 2022 16:24

ahoppen requested a review from rintaro January 14, 2022 16:29

[Code Completion] Extract NotRecommendedReason and CodeCompletionRes…

bad9679

…ultKind to top-level types Preliminary work to split `CodeCompletionResult` into a context-free and contextual part.

ahoppen force-pushed the pr/split-context-free-codecompletion-result branch from e0253ca to 6f65af5 Compare January 20, 2022 17:00

This was referenced Jan 25, 2022

[SourceKit] Compute code completion type relation when wrapping a ContextFreeCodeCompletionResult in a CodeCompletionResult #40999

Merged

[CodeComplete] Compute type relations for global cached results #41000

Merged

rintaro approved these changes Jan 27, 2022

View reviewed changes

ahoppen force-pushed the pr/split-context-free-codecompletion-result branch from 6f65af5 to fd72674 Compare January 27, 2022 09:41

ahoppen merged commit e8c76a1 into swiftlang:main Feb 8, 2022

ahoppen deleted the pr/split-context-free-codecompletion-result branch April 5, 2022 14:48

	/// Opaque kind that is interpreted differently depending on
	/// Opaque kind that is interpreted differently depending on 'Kind'.

[Code Completion] Split code completion results into context free part that can be cached and contextual part displayed to the user #40471

[Code Completion] Split code completion results into context free part that can be cached and contextual part displayed to the user #40471

Uh oh!

Conversation

ahoppen commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Dec 8, 2021

Uh oh!

rintaro Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

rintaro Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Dec 10, 2021

Uh oh!

ahoppen commented Dec 13, 2021

Uh oh!

ahoppen commented Dec 14, 2021

Uh oh!

ahoppen commented Dec 14, 2021

Uh oh!

rintaro Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jan 14, 2022

Uh oh!

ahoppen commented Jan 14, 2022

Uh oh!

ahoppen commented Jan 19, 2022

Uh oh!

ahoppen commented Jan 20, 2022

Uh oh!

ahoppen commented Jan 20, 2022

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

rintaro Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

ahoppen Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jan 27, 2022

Uh oh!

Uh oh!

ahoppen commented Dec 8, 2021 •

edited

Loading