Don't block completion for items from unimported namespaces by genlu · Pull Request #59045 · dotnet/roslyn

genlu · 2022-01-24T21:33:51Z

I have extracted the refactoring part in to a separate PR #59125, which hopefully would make code review at least slightly easier.

In addition to the changes in #59125, this PR divided the computation of completion items into two tasks:

"Core" items (i.e.non-expanded) which should be included in the list regardless of the selection of expander. Right now this includes all items except those from unimported namespaces.
Expanded items which only show in the completion list when expander is selected, or by default if the corresponding features are enabled. Right now only items from unimported namespaces are associated with expander.

(1) is the essence of completion so we'd always wait until its task is completed and return the results. However, because we have a really tight perf budget in completion, and computing those items in (2) could be expensive especially in a large solution (e.g.requires syntax / symbol indices and/ or runs in OOP,) we decide to kick off the computation in parallel when completion is triggered, but only include its results if it's completed by the time task (1) is completed, otherwise we don't wait on it and just return items from (1) immediately. Task (2) will still be running in the background (until session is dismissed/committed,) and we'd check back to see if it's completed whenever we have a chance to update the completion list, i.e.when user typed another character, a filter was selected, etc. If so, those items will be added as part of the refresh.

Update: this new behavior is now tied to "responsive completion" option, which is enabled by default. It means we'd not wait for unimported items to be calculated by default, but user can change it to "always wait" by disable "responsive completion"

The reason of adopting this approach is we want to minimize typing delays. There are two ways user might perceive a delay in typing. First, they could see a delay between typing a character and completion list being displayed if they want to examine the items available. Second, they might be typing continuously w/ o paying attention to completion list, and simply expect the completion to do the "right thing" when a commit char is typed(e.g.commit "cancellationToken" when typing 'can$TAB$'). However, the commit could be delayed if completion is still waiting on the computation of all available items, which manifests as UI delays and in worst case timeouts in commit which results in unexpected behavior(e.g. typing 'can$TAB$' results in a 250ms UI freeze and still ends up with "can" instead of "cancellationToken".)

This approach would ensure the computation of (2) will not be the cause of such delays, with the obvious trade off of potentially not providing expanded items until later(or never) in a completion session even if the feature is enabled. Note that in most cases we'd expect task (2) to finish in time and complete result would be available after typing one additional chracter after triggering, if not from the start of the session (this needs some further validation, if it turns out this is not true but only a short wait is what required to fix it, then we should consider it). However, even in the case only partial result is returned at the start, we still believe this is acceptable given how critical perf is in typing scenario. Additionally, expanded items are usually considered complementary. The need for them only rise occasionally (it's rare when users need to add imports,) and when they are needed, our hypothesis is because of their more intrusive nature(adding an import to the document) users would more likely to contemplate such action thus typing slower before commit and / or spending more time examining the list, which give us some opportunities to still provide those items later before they are truly required.

In this commit we have to handle adding those delayed expanded items into completion list in Roslyn. This is because the CompletionContext.IsIncomplete flag isn't fully supported in classic mode. Will need to move to that API once it's implemented properly.

todo:

add some telemetry
Make sure the cancellation for the background task is implemented properly

The computation of completion items is divided into two tasks: 1. "Core" items(i.e.non - expanded) which should be included in the list regardless of the selection of expander. Right now this includes all items except those from unimported namespaces. 2 .Expanded items which only show in the completion list when expander is selected, or by default if the corresponding features are enabled. Right now only items from unimported namespaces are associated with expander. #1 is the essence of completion so we'd always wait until its task is completed and return the results. However, because we have a really tight perf budget in completion, and computing those items in #2 could be expensive especially in a large solution (e.g.requires syntax / symbol indices and/ or runs in OOP,) we decide to kick off the computation in parallel when completion is triggered, but only include its results if it's completed by the time task #1 is completed, otherwise we don't wait on it and just return items from #1 immediately. Task #2 will still be running in the background (until session is dismissed/committed,) and we'd check back to see if it's completed whenever we have a chance to update the completion list, i.e.when user typed another character, a filter was selected, etc. If so, those items will be added as part of the refresh. The reason of adopting this approach is we want to minimize typing delays. There are two ways user might perceive a delay in typing. First, they could see a delay between typing a character and completion list being displayed if they want to examine the items available. Second, they might be typing continuously w/ o paying attention to completion list, and simply expect the completion to do the "right thing" when a commit char is typed(e.g.commit "cancellationToken" when typing 'can$TAB$'). However, the commit could be delayed if completion is still waiting on the computation of all available items, which manifests as UI delays and in worst case timeouts in commit which results in unexpected behavior(e.g.typing 'can$TAB$' results in a 250ms UI freeze and still ends up with "can" instead of "cancellationToken".) This approach would ensure the computation of #2 will not be the cause of such delays, with the obvious trade off of potentially not providing expanded items until later(or never) in a completion session even if the feature is enabled.Note that in most cases we'd expect task #2 to finish in time and complete result would be available from the start of the session.However, even in the case only partial result is returned at the start, we still believe this is acceptable given how critical perf is in typing scenario. Additionally, expanded items are usually considered complementary. The need for them only rise occasionally(it's rare when users need to add imports,) and when they are needed, our hypothesis is because of their more intrusive nature(adding an import to the document) users would more likely to contemplate such action thus typing slower before commit and / or spending more time examining the list, which give us some opportunities to still provide those items later before they are truly required. In this commit we have to handle adding those delayed expanded items into completion list in Roslyn. This is because the `CompletionContext.IsIncomplete` flag isn't fully supported in classic mode. Will need to move to that API once it's implemented properly.

Was broken by previous refactoring

So we can always wait for unimported items and force index creation

genlu · 2022-01-28T01:11:18Z

@CyrusNajmabadi @sharwell Please let me know if you have any concerns about this change to import completion.

sharwell · 2022-01-28T04:35:42Z

@genlu Remind me tomorrow to read through this with fresh eyes 😪

AmadeusW · 2022-01-29T00:52:06Z

This is a clever design.
It's safe in context of VS Completion because calls to CompletionSource and ItemManager are synchronized; only one is queried at a time. To verify, you can print thread ID in the debugger to verify that the same worker thread is doing it.

genlu · 2022-02-01T01:19:40Z

There's still a couple things I'd need to confirm/add, but I think the PR is ready for review.
PTAL @dotnet/roslyn-ide @sharwell @CyrusNajmabadi

sharwell · 2022-02-02T05:10:13Z

... but only include its results if it's completed by the time task (1) is completed ...

@genlu Is there a way to turn this off? The IDE keeps adding "features" to reduce the accuracy of IntelliSense lists and they are the absolute worst.

The only thing I care about here is predictability in the list. There is no UI delay long enough that it warrants invalidating the results what I'm typing. I need to be able to type part of an identifier, and know with certainty that after I press . the correct text will eventually appear. All performance concerns are secondary to the correctness requirement.

sharwell

This feature needs a UI option to disable it. When disabled, completion would always wait for both tasks to complete before considering all results available. An acceptable alternative is to bind it to the existing "Responsive code completion" editor option, which is enabled by default.

CyrusNajmabadi · 2022-02-02T17:04:40Z

Waiting on #59125 to go in.

CyrusNajmabadi · 2022-02-08T20:28:39Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

+            }
+            else if (session.Properties.TryGetProperty<ImmutableArray<VSCompletionItem>>(CombinedSortedList, out var combinedSortedList))
+            {
+                // Always use the previously saved combined list if available.


i don't understand ths... why are we even putting this into the properties... instead of just having this be in our model?

CyrusNajmabadi · 2022-02-11T20:10:29Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

+                AddPropertiesToSession(session, list, triggerLocation);
+                return context;
+            }
+            else if (!session.TextView.Options.GetOptionValue(DefaultOptions.ResponsiveCompletionOptionId))


dumb question. is this type safe to use on a BG thread? i won't block the PR on this, but can yuo check with amadeus?

Oh, that's a good point, I wasn't paying attention to the fact that it was called on UI thread.

Move it to UI thread just in case. Will check with editor team on this

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

sharwell · 2022-02-11T20:18:33Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/Helpers.cs

        /// </param>
        /// <returns>Roslyn completion trigger</returns>
-        internal static RoslynTrigger GetRoslynTrigger(EditorAsyncCompletionData.CompletionTrigger trigger, SnapshotPoint triggerLocation)
+        public static RoslynTrigger GetRoslynTrigger(EditorAsyncCompletionData.CompletionTrigger trigger, SnapshotPoint triggerLocation)


😕 Why are these changing? This file should have worked the same without any changes.

I initially made some change in this file which was later deleted. And I changed accessibility to make them consistent with other place in the code while doing so (but did not revert it after)

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

dibarbet · 2022-02-11T19:52:23Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/AsyncCompletionLogger.cs

-            // For type import completion
+            // For import completion
            SessionWithTypeImportCompletionEnabled,
+            SessionWithImportCompletionBlocking,


some comments on these might be useful. For example is SessionWithImportCompletionBlocking mean that type import completion completed by the time the normal completion items were returned?

And does SessionWithDelayedImportCompletionIncludedInUpdate count the number of times that delayed import completion items were added later? So to determine the number of times import completion failed to be included, we would subtract that value from SessionWithImportCompletionDelayed?

SessionWithImportCompletionBlocking records the number of sessions where import completion is enabled and we wait for them to complete (because the editor's responsive completion option is disabled). Subtracting it from SessionWithTypeImportCompletionEnabled gives us the number of sessions the delay could happen (if import completion is slow.)

You are correct about the other two :)

dibarbet · 2022-02-11T19:56:41Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

-                return AsyncCompletionData.CompletionContext.Empty;
+                // OK, expand item is enabled but we shouldn't block completion on its results.
+                // Kick off expand item calculation first in background.
+                Stopwatch stopwatch = new();


would this type be at all useful here to avoid creating a stopwatch each time?
https://sourceroslyn.io/#Microsoft.CodeAnalysis/InternalUtilities/SharedStopwatch.cs,12

❗ Yes this needs to use SharedStopwatch. ~~Also I thought we had a code style rule employed to prevent creating new value types with new().~~ (Stopwatch is a reference type)

Hmm, then we probably need additional flag and lock since we might check the elapsed time before it even started.

dibarbet · 2022-02-11T20:06:54Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

+                    // Expanded item task still running. Save it to the session and return non-expanded items immediately.
+                    // Also start the stopwatch since we'd like to know how long it takes for the expand task to finish
+                    // after core providers completed (instead of how long it takes end-to-end).
+                    stopwatch.Start();


This doesn't really affect the user experience, but this can race with the read in expandedItemsTask right?
Because if the stopwatch is started after the if elapsedMs > 0 then we record a delayed completion event, but don't get the number of ticks delayed in telemetry.

Would it be easier to just record both the normal completion task time and the expanded completion task time, then when logging telemetry just wait for both to finish and calculate the diff?

Yes, there'd be race, I ignored that since I didn't think we need high level of accuracy for this. Now that you mentioned it, I think it'd probably better to log it regardless of whether stopwatch is started (remove the >0 check), which would give us a better idea as of how much longer import completion task takes (even if it's finished almost in time but ended up not included.)

The issue with recording time for both tasks is we trigger the import completion task first, and we are more interested in the additional time needed after the regular task completed. Maybe it doesn't matter, I'm not sure

Ah - so in the cases where it finishes before you just record a 0 additional time? That seems to make sense and would be straightforward.

Yup, and we have another property to tell us how many time it's delayed

dibarbet · 2022-02-11T20:11:36Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

+                    session.Properties.RemoveProperty(ExpandedItemsTask);
+                    var (expandedContext, expandedCompletionList) = await expandedItemsTask.ConfigureAwait(false);
+                    AddPropertiesToSession(session, expandedCompletionList, triggerLocation: null);
+                    return expandedContext;


taht this is a Get method that mutates properties is a bit oogy. can the data just flow outwards, and someone else can be responsible for this?

+1 from me

i'm not really sure i get why we return the expandedContext but adding the expactedComlpetionList as a property to the session...

This was confusing to me as well - but I think the expanded completion list is not actually being added to the properties bag here, right? Its just taking certain properties from the list and adding them. It might be good to explicitly pass in what you're adding to the properties bag?

dibarbet · 2022-02-11T20:28:36Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

+                    var combinedList = itemsBuilder.ToImmutable();
+
+                    // Add expanded items into a combined list, and save it to be used for future updates during the same session.
+                    session.Properties[CombinedSortedList] = combinedList;


This is probably just my unfamiliarity with how roslyn completion is handling this - is the session properties bag the only way to pass data between calls for the same session? Is there an analogous type on the roslyn side that we could just put it on to retrieve later?

genlu · 2022-02-11T21:01:10Z

Its just taking certain properties from the list and adding them.

that's correct.

Is there an analogous type on the roslyn side that we could just put it on to retrieve later?

I have filed #59262 for this. Maybe we can use a CWT instead of those property bags? I definitely need to better understand editor code first.

sharwell

Two more files left to review ...

src/Features/Core/Portable/Completion/CompletionItem.cs

sharwell · 2022-02-11T20:28:39Z

src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests.vb

+            End Sub
+
+            Public Overrides Async Function ProvideCompletionsAsync(context As CompletionContext) As Task
+                Await checkpoint.Task.ConfigureAwait(False)


😕 Just use ManualResetEventSlim?

sharwell · 2022-02-11T20:34:16Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

+                    var combinedList = itemsBuilder.MoveToImmutable();
+
+                    // Add expanded items into a combined list, and save it to be used for future updates during the same session.
+                    session.Properties[CombinedSortedList] = combinedList;


😕 Does it not retain the previous filter? We have to add them each time?

The FilterStates we returned from this update would be retained by the model in editor, and subsequent call to update would pass the combined one to us, so we only need to remember the combined IntialList (which is created when session is first triggered and doesn't change over updates)

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs

sharwell · 2022-02-12T00:40:29Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

+                {
+                    // the task of expanded item is completed, get the result and combine it with result of non-expanded items.
+                    var (expandedContext, expandedCompletionList) = await expandedItemsTask.ConfigureAwait(false);
+                    AddPropertiesToSession(session, expandedCompletionList, triggerLocation);


Why do we call AddPropertiesToSession here when we already called it a few lines above?

sharwell · 2022-02-12T00:44:54Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

+        }

-            static void AddPropertiesToSession(IAsyncCompletionSession session, CompletionList completionList, SnapshotPoint triggerLocation)
+        private static void AddPropertiesToSession(IAsyncCompletionSession session, CompletionList completionList, SnapshotPoint triggerLocation)


💡 Can you add a comment on this explaining in fair detail what its purpose is?

➡️ Looks like this didn't change as part of this PR (just indentation).

I will leave it as is since I'm hoping to get rid the property bag usage in a follow up.

ghost added the Area-IDE label Jan 24, 2022

genlu force-pushed the MultiPhaseCompletion branch from 3094fc3 to ccf660a Compare January 24, 2022 21:39

genlu added 2 commits January 26, 2022 17:34

Handling cancellation of the task for expanded items

78b99bf

runfoapp bot mentioned this pull request Jan 27, 2022

InitializeToolset flakiness in correctness builds #58199

Closed

genlu added 4 commits January 27, 2022 14:24

Merge remote-tracking branch 'dotnet/main' into MultiPhaseCompletion

9c32638

Fix completion trigger

fa9d355

Was broken by previous refactoring

Return partial result for unimported types completion when cache miss

d9f34da

Add test only options for import completion

64a778f

So we can always wait for unimported items and force index creation

genlu changed the title ~~[Draft] import completion simplification~~ [Draft] Don't block completion for items from unimported namespaces Jan 27, 2022

genlu mentioned this pull request Jan 28, 2022

CompletionService refactoring and small tweaks to import completion #59125

Merged

Clean up telemetry

4b8924b

runfoapp bot mentioned this pull request Jan 28, 2022

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

genlu added 3 commits January 31, 2022 12:15

Merge branch 'CompletionRefactoring' into MultiPhaseCompletion

f468fee

Trigger expand item task first

aa8f49e

Add tests for two-phase completion

2796eac

genlu marked this pull request as ready for review February 1, 2022 01:18

genlu requested review from a team as code owners February 1, 2022 01:18

genlu changed the title ~~[Draft] Don't block completion for items from unimported namespaces~~ Don't block completion for items from unimported namespaces Feb 1, 2022

genlu force-pushed the MultiPhaseCompletion branch from d1a8e7e to c5f5314 Compare February 1, 2022 23:47

Add telemetry for delaying import completion

742bb0f

genlu force-pushed the MultiPhaseCompletion branch from c5f5314 to 742bb0f Compare February 2, 2022 01:32

sharwell suggested changes Feb 2, 2022

View reviewed changes

CyrusNajmabadi reviewed Feb 8, 2022

View reviewed changes

genlu added 2 commits February 8, 2022 16:01

Don't lock stopwatch

eeb3e10

Address review comments

7f715a6

CyrusNajmabadi reviewed Feb 11, 2022

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs Outdated Show resolved Hide resolved

CyrusNajmabadi approved these changes Feb 11, 2022

View reviewed changes

sharwell reviewed Feb 11, 2022

View reviewed changes

dibarbet reviewed Feb 11, 2022

View reviewed changes

genlu added 2 commits February 11, 2022 13:48

Merge remote-tracking branch 'dotnet/main' into MultiPhaseCompletion

72021f1

Get editor options on UI thread

e2f822c

sharwell reviewed Feb 11, 2022

View reviewed changes

Fix telemetry logging and add comments

f210b91

sharwell reviewed Feb 11, 2022

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs Show resolved Hide resolved

sharwell reviewed Feb 11, 2022

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 11, 2022

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs Outdated Show resolved Hide resolved

Address review comments

990e7d5

sharwell reviewed Feb 12, 2022

View reviewed changes

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 12, 2022

View reviewed changes

sharwell approved these changes Feb 12, 2022

View reviewed changes

genlu added 2 commits February 11, 2022 17:16

Add a test to ensure completion list ordering

f22310b

Change if statements order

f327718

genlu enabled auto-merge February 12, 2022 01:26

genlu merged commit 5193908 into dotnet:main Feb 12, 2022

ghost added this to the Next milestone Feb 12, 2022

genlu deleted the MultiPhaseCompletion branch February 12, 2022 22:00

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022

genlu mentioned this pull request Mar 8, 2022

Eliminate more instances of IWorkspaceOptionsProvider OmniSharp/omnisharp-roslyn#2343

Merged

Conversation

genlu commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

genlu commented Jan 28, 2022

Uh oh!

sharwell commented Jan 28, 2022

Uh oh!

AmadeusW commented Jan 29, 2022

Uh oh!

genlu commented Feb 1, 2022

Uh oh!

sharwell commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu commented Feb 11, 2022

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

genlu commented Jan 24, 2022 •

edited

Loading

sharwell commented Feb 2, 2022 •

edited

Loading

sharwell left a comment •

edited

Loading

sharwell Feb 11, 2022 •

edited

Loading

genlu Feb 11, 2022 •

edited

Loading

sharwell Feb 12, 2022 •

edited

Loading