Switch go-to-impl to be entirely async/non-blocking. #57861

CyrusNajmabadi · 2021-11-18T19:12:53Z

Fixes AB#1419331
Fixes #41374

The prior approach to go-to-impl was:

get the command from teh editor (already under a threaded wait dialog 'TWD').
perform the search in a blocking (but cancellable) fashion while the TWD was up.
once the results were found, either:
1. pop up message if no results found
2. navigate to the result if there was only one result found
3. show find-refs-window if multiple results were found

This approach is unfortunate as it blocks all interaction with VS while the search is going on. As finding impls can be expensive (they can be anywhere in the solution) this can mean waiting a very long time.

The new approach is as follows.

Immediately return from the editor command, so no TWD is present
Kick off the work to find results asynchronously.
If the results come back in less than 1.5 seconds (delay at which TWD normally appears, and operations are considered to be 'long running') then do the same as the prior approach and either navigate, or present.
If the results are taking more than 1.5 seconds, then unilaterally pop up the find-refs-window to indicate that the search is ongoing. From taht point on, no matter what hte search results (0, 1 or multiple), the results are put into the window. This way we do not interfere with typing/navigation by popping up dialogs or navigating the user.

This now allows the user to continue doing whatever they want while the operations are in progress. Similar to FInd-Refs, they are not blocked in any fashion.

Another way to think about this is that we have almost the same experience as Find-Refs, except:

if we find only a single result
and we find that single result in <1.5 seconds
then we navigate to that item immediately

--

Note: this PR does not move us to a point where go-to-impl results are streamed in. That woudl be very nice, but is currently problematic as the current go-to-impl algorithm needs to find everything in order to filter out parts it may not want to show based on the full set of symbols found. We can consider revisiting that approach if it we think it would be better to just present results as we find them so that hte user can interact with them (like we do with FAR).

--

This PR also updates the feature to not block on things while the solution is loading, but instead do tings best effort. In the event that the solution is loading while hte search is performed, teh user will see:

This follows a similar message that comes up in nav-to.

CyrusNajmabadi · 2021-11-18T19:13:20Z

src/EditorFeatures/Core/CommandHandlers/AbstractGoToCommandHandler.cs

@@ -2,132 +2,263 @@
 // The .NET Foundation licenses this file to you under the MIT license.


this type is substantially rewritten. i'll try to comment what is going on for clarity.

CyrusNajmabadi · 2021-11-18T19:22:39Z

src/EditorFeatures/Core/FindUsages/AbstractFindUsagesService_FindImplementations.cs

+            await context.SetSearchTitleAsync(
+                string.Format(EditorFeaturesResources._0_implementations,
+                FindUsagesHelpers.GetDisplayName(symbol)),
+                cancellationToken).ConfigureAwait(false);


moved this to be first so that the title of the FAR window properly says "IFoo" implementations even if we end up finding nothing.

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

…ts/AbstractTableDataSourceFindUsagesContext.cs

CyrusNajmabadi · 2021-11-18T19:23:47Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

+            /// <summary>
+            /// Message we show if we find no definitions.  Consumers of the streaming presenter can set their own title.
+            /// </summary>
+            protected string NoDefinitionsFoundMessage = ServicesVSResources.Search_found_no_results;


we used to hardcode this. now we just support the contract we always had before (but this type ignored).

CyrusNajmabadi · 2021-11-18T19:24:41Z

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs

@@ -414,7 +419,14 @@ protected RoslynDefinitionBucket GetOrCreateDefinitionBucket(DefinitionItem defi
            }

            public sealed override ValueTask ReportMessageAsync(string message, CancellationToken cancellationToken)
-                => throw new InvalidOperationException("This should never be called in the streaming case.");


this can now be called in the streaming case. in streaming-go-to-impl, we may end up finding no results. so we want to show that message to the user in the FAR window. We could just use the default string, but since hte API allows the caller to specify the message to show hte user, we should respect it.

…oslyn into gotoImplPumping

jasonmalinowski · 2021-11-19T01:15:51Z

@CyrusNajmabadi As far as the experience goes, do we have a way to give the user some indication we're doing something before the 1.5 second cutoff? My worry is if I hit the key and then still see my cursor flashing, I'm going to think that I didn't do anything in the first place. Wondering if there's a potential alternate approach which is something more like:

Pop-up the tool window, always.
If the result populates with a single result, and I hadn't clicked anywhere else or moved the caret or something, then close it and navigate if there's a single hit.

That way there's always visual indication we're doing something, and the I'm not paying a behavior change penalty if my search happened to take 1.6 seconds.

(I won't pretend that condition of the second step will be easy to implement....)

CyrusNajmabadi · 2021-11-19T04:16:38Z

@jasonmalinowski I don't think we need the indicator. There's already this amount of time until the twd would normally appear. So both before and after there's a small delay with nothing happening before you get the UI element which let's you know that long running work is happening

CyrusNajmabadi · 2021-11-19T04:41:28Z

Pop-up the tool window, always.

Also, 8 really don't like that. It feels like a unfortunate ui for a case that will happen a lot.

davidwengier · 2021-11-19T06:32:51Z

The Background work indicator would be a good fit here.

CyrusNajmabadi · 2021-11-19T07:07:33Z

Note: to be clear, there is an indication that background work is being done. You get the same indication that you get with find-refs. Namely a progress indicator in the FAR window :-)

We coudl still decide to show a BackgroundWorkIndicator immediately if we thought that would help.

dibarbet

would +1 @davidwengier suggestion to use that UI once available (for the pre-1.5s time)

dibarbet · 2021-11-19T20:00:25Z

src/EditorFeatures/Core/CommandHandlers/AbstractGoToCommandHandler`2.cs

+    /// if the user kicks off another actual go-to-impl command.  In that case, we just attempt to cancel the prior
+    /// command (if it is still running), then wait for it to complete, then run our command.  The second is if we have
+    /// switched over to the streaming presenter and then the user starts some other command (like FAR) that takes over
+    /// the presenter.  In that case, the presenter will notify us that it has be re-purposed and we will also cancel


once we've computed the items and put them in the presenter, why is there anything that needs to be cancelled here if another presenter takes over? That seems a bit strange - it seems like the other presenter should just take over, and the existing one is 'disposed' of

So, there's only one FAR window (unless you select 'keep results' in the window). So once another feature wants to push results into that window we want to stop what we're doing as there's no point anymore.

dibarbet · 2021-11-19T20:08:31Z

src/EditorFeatures/Core/CommandHandlers/AbstractGoToCommandHandler`2.cs

+        var findContext = new BufferedFindUsagesContext();
+
+        var cancellationToken = cancellationTokenSource.Token;
+        var delayTask = Task.Delay(TaggerDelay.OnIdle.ComputeTimeDelay(), cancellationToken);


hmm I think I'd prefer to not use the tagger delay since this doesnt really have anything to do with tagging. I don't think we'd want this changed if we changed the idle tagger delay for tagging reasons

so, 'TaggerDelay' is poorly named. It should really be called something like 'PerceptionConstants'. These are values provided form research from teh platform/UX teams about how people perceive delays. i.e. what a timespan needs to be to be considered with the 'instantaneous' threshold for people, etc.

This timespan (which we use in many places outside of tagging) is the: enough time has passed for me to start realizing it's going to take a while to get results, so transition the UI to let hte user know.

dibarbet · 2021-11-19T20:52:27Z

src/EditorFeatures/Core/CommandHandlers/AbstractGoToCommandHandler`2.cs

+    {
+        var cancellationToken = cancellationTokenSource.Token;
+        await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken);
+        var (presenterContext, presenterCancellationToken) = _streamingPresenter.StartSearch(this.DisplayName, supportsReferences: false);


I wonder if this could be made cleaner by allowing the IStreamingFindReferencesPresenter to take in an optional find usages context that it uses. Then it seems like you wouldn't need all the complexity in BufferedFindUsagesContext to switch between the presenters

It wouldn't work. The FindContext is what features push into. The window itself (which is the context actually), needs to pull the already computed values.

dibarbet · 2021-11-19T20:55:04Z

src/EditorFeatures/Core/CommandHandlers/BufferedFindUsagesContext.cs

+
+    public async Task<string?> GetMessageAsync(CancellationToken cancellationToken)
+    {
+        using (await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))


seems like all these could be simple using statements

Yes. I can change that

src/EditorFeatures/Core/CommandHandlers/BufferedFindUsagesContext.cs

src/VisualStudio/Core/Def/Implementation/FindReferences/StreamingFindUsagesPresenter.cs

…ingFindUsagesPresenter.cs Co-authored-by: Marius Ungureanu <[email protected]>

CyrusNajmabadi · 2021-11-21T23:53:51Z

Tagging @davidwengier

davidwengier · 2021-11-22T01:29:47Z

src/EditorFeatures/Core/CommandHandlers/AbstractGoToCommandHandler`2.cs

+
+        var cancellationToken = cancellationTokenSource.Token;
+        var delayTask = Task.Delay(TaggerDelay.OnIdle.ComputeTimeDelay(), cancellationToken);
+        var findTask = Task.Run(() => FindResultsAsync(document, position, findContext, cancellationToken), cancellationToken);


For my curiosity: Is the Task.Run needed here, if we're already on a background thread due to the TaskScheduler.Default?

It likely isn't. i can remove in a followup :)

davidwengier · 2021-11-22T01:45:05Z

src/VisualStudio/Core/Def/Implementation/FindReferences/StreamingFindUsagesPresenter.cs

+            var guid = new Guid("a80febb4-e7e0-4147-b476-21aaf2453969");
+
+            if (_serviceProvider.GetService(typeof(SVsUIShell)) is not IVsUIShell uiShell ||
+                uiShell.FindToolWindow((uint)__VSFINDTOOLWIN.FTW_fFindFirst, ref guid, out var windowFrame) != VSConstants.S_OK ||


Doesn't look like there is an FTW_fFindLast, but does this do the right thing when Keep Results is used?

This is a good question. I tried combinations of tings to see if i could break this, but it always seemed to work. So, afaict, this is ok... but i'm not sure why TBH :)

CyrusNajmabadi added 8 commits November 17, 2021 21:34

Release the UI thread when performing GoToImpl command

3ba2fd7

Formatting

60ebb32

About to undo

332dee4

Fixes

edf0c28

Entirely async goto-impl

cd9fa01

Extract file

4c11943

Fix integration harness

0d0ae01

UI improvements

dab14d5

CyrusNajmabadi requested review from jasonmalinowski and dibarbet November 18, 2021 19:12

CyrusNajmabadi requested a review from a team as a code owner November 18, 2021 19:12

dotnet-issue-labeler bot added the Area-IDE label Nov 18, 2021

CyrusNajmabadi commented Nov 18, 2021

View reviewed changes

CyrusNajmabadi added 2 commits November 18, 2021 11:14

Simplify

cf39c6e

Let the user know if we're waiting on the solution to load

f4db61c

CyrusNajmabadi requested a review from a team as a code owner November 18, 2021 19:21

CyrusNajmabadi commented Nov 18, 2021

View reviewed changes

.../Core/Def/Implementation/FindReferences/Contexts/AbstractTableDataSourceFindUsagesContext.cs Outdated Show resolved Hide resolved

Update src/VisualStudio/Core/Def/Implementation/FindReferences/Contex…

b9727d4

…ts/AbstractTableDataSourceFindUsagesContext.cs

CyrusNajmabadi commented Nov 18, 2021

View reviewed changes

CyrusNajmabadi added 9 commits November 18, 2021 11:26

Add comments

9c71338

Add comments

8caed62

Remove stale comment

32c03f1

Wrap

4edd1f7

Give the user a message if the solution is not fully loaded.

269304f

Rvert

c869f77

REvert

62da7bb

Add loc string

d333ed4

Pass information along even when popping up a message

7f443d7

CyrusNajmabadi added 7 commits November 18, 2021 14:52

Add ocmment

ad49244

Merge branch 'gotoImplPumping' of https://github.com/CyrusNajmabadi/r…

cc70f1e

…oslyn into gotoImplPumping

Simplify

fbc556a

Add contract

be9c09b

Simplify

4cae7d2

revert

aaec370

remove copy

df2586f

CyrusNajmabadi requested a review from davidwengier November 19, 2021 00:29

dibarbet reviewed Nov 19, 2021

View reviewed changes

Use simple using statement

703bb7e

CyrusNajmabadi force-pushed the gotoImplPumping branch from e492eb2 to 703bb7e Compare November 20, 2021 20:15

Use nrt and an internal state object

a22525d

Therzok reviewed Nov 21, 2021

View reviewed changes

src/VisualStudio/Core/Def/Implementation/FindReferences/StreamingFindUsagesPresenter.cs Outdated Show resolved Hide resolved

CyrusNajmabadi requested a review from dibarbet November 21, 2021 21:12

Update src/VisualStudio/Core/Def/Implementation/FindReferences/Stream…

88fdda9

…ingFindUsagesPresenter.cs Co-authored-by: Marius Ungureanu <[email protected]>

davidwengier approved these changes Nov 22, 2021

View reviewed changes

CyrusNajmabadi merged commit 2e26701 into dotnet:main Nov 22, 2021

ghost added this to the Next milestone Nov 22, 2021

CyrusNajmabadi deleted the gotoImplPumping branch November 22, 2021 02:11

runfoapp bot mentioned this pull request Nov 22, 2021

Flaky test: Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpFindReferences.FindReferencesToCtor #57552

Closed

allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021

CyrusNajmabadi mentioned this pull request Dec 22, 2021

Rivisit 'async go to impl'. #58467

Closed

sharwell mentioned this pull request Mar 4, 2022

CSharpGoToImplementation.SimpleGoToImplementation test is not deterministic #59950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch go-to-impl to be entirely async/non-blocking. #57861

Switch go-to-impl to be entirely async/non-blocking. #57861

CyrusNajmabadi commented Nov 18, 2021 •

edited by Pilchie

Loading

CyrusNajmabadi Nov 18, 2021

CyrusNajmabadi Nov 18, 2021

CyrusNajmabadi Nov 18, 2021

CyrusNajmabadi Nov 18, 2021

jasonmalinowski commented Nov 19, 2021 •

edited

Loading

CyrusNajmabadi commented Nov 19, 2021

CyrusNajmabadi commented Nov 19, 2021

davidwengier commented Nov 19, 2021

CyrusNajmabadi commented Nov 19, 2021 •

edited

Loading

dibarbet left a comment •

edited

Loading

dibarbet Nov 19, 2021 •

edited

Loading

CyrusNajmabadi Nov 20, 2021

dibarbet Nov 19, 2021

CyrusNajmabadi Nov 20, 2021

dibarbet Nov 19, 2021

CyrusNajmabadi Nov 20, 2021

dibarbet Nov 19, 2021

CyrusNajmabadi Nov 20, 2021

CyrusNajmabadi commented Nov 21, 2021

davidwengier Nov 22, 2021

CyrusNajmabadi Nov 22, 2021

davidwengier Nov 22, 2021

CyrusNajmabadi Nov 22, 2021

		@@ -2,132 +2,263 @@
		// The .NET Foundation licenses this file to you under the MIT license.

Switch go-to-impl to be entirely async/non-blocking. #57861

Switch go-to-impl to be entirely async/non-blocking. #57861

Conversation

CyrusNajmabadi commented Nov 18, 2021 • edited by Pilchie Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Nov 19, 2021 • edited Loading

CyrusNajmabadi commented Nov 19, 2021

CyrusNajmabadi commented Nov 19, 2021

davidwengier commented Nov 19, 2021

CyrusNajmabadi commented Nov 19, 2021 • edited Loading

dibarbet left a comment • edited Loading

Choose a reason for hiding this comment

dibarbet Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 18, 2021 •

edited by Pilchie

Loading

jasonmalinowski commented Nov 19, 2021 •

edited

Loading

CyrusNajmabadi commented Nov 19, 2021 •

edited

Loading

dibarbet left a comment •

edited

Loading

dibarbet Nov 19, 2021 •

edited

Loading