Always try to use cached data for import completion even if outdated by genlu · Pull Request #59760 · dotnet/roslyn

genlu · 2022-02-25T02:18:03Z

And refresh cache in background.

Even if it might be stale. the completion list would be computed with cached data and cache refresh would be done in background.

…pletion is triggered

...e/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

genlu · 2022-02-28T20:34:58Z

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

-            var referencedProjects = graph.GetProjectsThatThisProjectTransitivelyDependsOn(currentProject.Id).SelectAsArray(id => solution.GetRequiredProject(id));
+            var referencedProjects = graph.GetProjectsThatThisProjectTransitivelyDependsOn(currentProject.Id).Select(id => solution.GetRequiredProject(id)).Where(p => p.SupportsCompilation);
+
+            projectsBuilder.Add(currentProject);


Note that we no longer have any special treatment for current project, i.e. we return whatever in the cache, or if the cache isn't ready, we don't return anything from it.

CyrusNajmabadi · 2022-02-28T23:45:45Z

Checking.

CyrusNajmabadi · 2022-02-28T23:48:04Z

...e/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs

            {
+                // Trigger a backgound task to warm up cache and return immediately.
+                // This won't cause multiple cache update to run simultaneously because WarmUpCacheAsync is implemented by AsyncBatchingWorkQueue.
+                _ = Task.Run(() => WarmUpCacheAsync(document));


You shouldn't need the Task.Run. You shoudl just add an item to the work queue.

CyrusNajmabadi · 2022-02-28T23:51:08Z

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

+                return Task.CompletedTask;
+
+            _workQueue.AddWork(project.Id);
+            return _workQueue.WaitUntilCurrentBatchCompletesAsync();


you definitely don't want to wait on the work to complete :) that somewhat defeats the purpose. you just want to put the item in the queue and then know that it will be taken care of later.

This is the code path that returns the cache update task, not the code path for completion. That said, this can definitely use a little cleanup to make things clearer. Will update

could you point me to the code that looks at the return value of this method?

CyrusNajmabadi · 2022-03-01T00:30:01Z

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs


-            static bool HasGlobalAlias(MetadataReference? metadataReference)
-                => metadataReference != null && (metadataReference.Properties.Aliases.IsEmpty || metadataReference.Properties.Aliases.Any(alias => alias == MetadataReferenceProperties.GlobalAlias));
+            return (resultBuilder.ToImmutable(), isPartialResult);


just curious what you do with isPArtialResult on the consumption side.

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

CyrusNajmabadi · 2022-03-01T00:34:19Z

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

+        }
+
+        private static bool HasGlobalAlias(ImmutableArray<string> aliases)
+            => aliases.IsEmpty || aliases.Any(alias => alias == MetadataReferenceProperties.GlobalAlias);


this needs doc'ing. why does 'aliases.IsEmpty' imply you have global aliases?

CyrusNajmabadi · 2022-03-01T00:35:28Z

...ortCompletionProvider/ExtensionMethodImportCompletionHelper.ExtensionMethodSymbolComputer.cs

+                    foreach (var peReference in GetAllRelevantPeReferences(project))
+                        await SymbolTreeInfo.GetInfoForMetadataReferenceAsync(project.Solution, peReference, loadOnly: false, cancellationToken).ConfigureAwait(false);
                }
+            }


does thsi run in oop? or in VS? If in OOP, i'm ok with this running in parallel over all the PERefs. Unless you want it serial? (i can see args both ways).

This is in OOP (if enabled, that is)

...ortCompletionProvider/ExtensionMethodImportCompletionHelper.ExtensionMethodSymbolComputer.cs

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

src/Tools/ExternalAccess/OmniSharp/Completion/OmniSharpCompletionOptions.cs

sharwell

Missing async tracking

...letion/Providers/ImportCompletionProvider/AbstractExtensionMethodImportCompletionProvider.cs

...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs

src/Tools/ExternalAccess/OmniSharp/Completion/OmniSharpCompletionOptions.cs

…ionOptions.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

genlu · 2022-03-15T20:08:10Z

...Core/Portable/Completion/Providers/ImportCompletionProvider/IImportCompletionCacheService.cs


        IDictionary<ProjectId, TProject> ProjectItemsCache { get; }
+
+        CancellationToken DisposalToken { get; }


All the change in the last commits are refactoring except this and its implementation.

genlu · 2022-03-15T20:09:03Z

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

+                   TimeSpan.FromSeconds(1),
+                   BatchUpdateCacheAsync,
+                   listener,
+                   CacheService.DisposalToken);


Use token for workqueue

genlu · 2022-03-15T20:09:12Z

...n/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.SymbolComputer.cs

+                                TimeSpan.FromSeconds(1),
+                                BatchUpdateCacheAsync,
+                                listenerProvider.GetListener(FeatureAttribute.CompletionSet),
+                                cacheService.DisposalToken);


Use token for workqueue

genlu · 2022-03-15T20:10:10Z

...nse/ImportCompletionCacheService/EditorExtensionMethodImportCompletionCacheServiceFactory.cs

+    // This is the implementation at Editor layer to provide a CancellationToken
+    // for the workqueue used for background cache refresh.
+    [ExportWorkspaceServiceFactory(typeof(IImportCompletionCacheService<ExtensionMethodImportCompletionCacheEntry, object>), ServiceLayer.Editor), Shared]
+    internal sealed class EditorExtensionMethodImportCompletionCacheServiceFactory


new Editor layer factory

genlu · 2022-03-15T20:10:16Z

...e/IntelliSense/ImportCompletionCacheService/EditorTypeImportCompletionCacheServiceFactory.cs

+{
+    // This is the implementation at Editor layer to provide a CancellationToken
+    // for the workqueue used for background cache refresh.
+    [ExportWorkspaceServiceFactory(typeof(IImportCompletionCacheService<TypeImportCompletionCacheEntry, TypeImportCompletionCacheEntry>), ServiceLayer.Editor), Shared]


new Editor layer factory

genlu · 2022-03-16T18:02:45Z

Hey @CyrusNajmabadi, another look at this please?

...e/IntelliSense/ImportCompletionCacheService/EditorTypeImportCompletionCacheServiceFactory.cs

...n/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.SymbolComputer.cs

CyrusNajmabadi · 2022-03-18T20:27:16Z

...ore/Portable/Completion/Providers/ImportCompletionProvider/TypeImportCompletionCacheEntry.cs

+            }
+        }
+
+        public class Builder : IDisposable


do you pass your builders around? you coudl consider these being structs too. if you do pass it around, yuo will need to pass by ref given the mutable publicItemCount.

sharwell · 2022-03-21T20:58:04Z

...ortCompletionProvider/ExtensionMethodImportCompletionHelper.ExtensionMethodSymbolComputer.cs

+                        var exportProvider = (IMefHostExportProvider)project.Solution.Workspace.Services.HostServices;
+                        var listenerProvider = exportProvider.GetExports<AsynchronousOperationListenerProvider>().Single().Value;


📝 Code shouldn't assume HostServices implements IMefHostExportProvider. Can we not pass the listener to the constructor of this type?

You are commenting on an out-of-date snapshot, this is moved to https://github.com/dotnet/roslyn/pull/59760/files#diff-91071542ca3996ad4d7178bc276d40deaae3e2a3b7522531f29d869e1fb636ccR48

That said, I'm still using the same code to fet listener provider there. I assume I can still import the provider in the constructor safely?

Dismissed since original concern (as well as subsequent comments) have been addressed

genlu added 6 commits February 9, 2022 14:21

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

427d9ea

Merge branch 'sessionData' into MultiPhaseCompletion2

ce6a216

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

d84cca7

Use cached data for completion of unimported types

e65f7fd

Even if it might be stale. the completion list would be computed with cached data and cache refresh would be done in background.

Use cached data for completion of unimported extension methods

67c6dad

Even if it might be stale. the completion list would be computed with cached data and cache refresh would be done in background.

Try to refresh import completion cache in the background whenever com…

c635be6

…pletion is triggered

genlu requested a review from a team as a code owner February 25, 2022 02:18

ghost added the Area-IDE label Feb 25, 2022

CyrusNajmabadi reviewed Feb 25, 2022

View reviewed changes

...e/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 25, 2022

View reviewed changes

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs Outdated Show resolved Hide resolved

genlu added 2 commits February 25, 2022 18:01

Use AsyncBatchingWorkQueue for background cache updates

bae71ac

Fix tests

e663da0

genlu commented Feb 28, 2022

View reviewed changes

genlu requested a review from a team February 28, 2022 20:36

CyrusNajmabadi reviewed Feb 28, 2022

View reviewed changes

CyrusNajmabadi reviewed Mar 1, 2022

View reviewed changes

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 1, 2022

View reviewed changes

...ortCompletionProvider/ExtensionMethodImportCompletionHelper.ExtensionMethodSymbolComputer.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 1, 2022

View reviewed changes

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs Outdated Show resolved Hide resolved

genlu added 2 commits February 28, 2022 18:05

clean up

9785979

Fix tests

9053574

genlu requested a review from a team as a code owner March 3, 2022 01:52

Make sure background task to update cache always run

f8f9782

genlu force-pushed the UseCache branch from 120d90f to f8f9782 Compare March 3, 2022 01:53

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

cd6c665

genlu commented Mar 3, 2022

View reviewed changes

src/Tools/ExternalAccess/OmniSharp/Completion/OmniSharpCompletionOptions.cs Outdated Show resolved Hide resolved

sharwell previously requested changes Mar 9, 2022

View reviewed changes

Use AsynchronousOperationListener for background workqueues

205766f

genlu force-pushed the UseCache branch from 93bcbcf to 205766f Compare March 10, 2022 18:52

Update src/Tools/ExternalAccess/OmniSharp/Completion/OmniSharpComplet…

f77a0ad

…ionOptions.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

This was referenced Mar 10, 2022

Failure in CSharpCodeActions.GFUFuzzyMatchAfterRenameTrackingAndAfterGenerateType #57423

Closed

FixAllOccurrencesIgnoresGeneratedCode is flaky #56625

Closed

genlu requested a review from sharwell March 10, 2022 22:23

genlu added 2 commits March 15, 2022 11:46

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

534dca0

Pass ThreadingContext.DisposalToken to workqueue for cache refresh

5ecff82

genlu commented Mar 15, 2022

View reviewed changes

Remove namespace imports

c44946e

CyrusNajmabadi reviewed Mar 18, 2022

View reviewed changes

...e/IntelliSense/ImportCompletionCacheService/EditorTypeImportCompletionCacheServiceFactory.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 18, 2022

View reviewed changes

...n/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.SymbolComputer.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Mar 18, 2022

View reviewed changes

CyrusNajmabadi approved these changes Mar 18, 2022

View reviewed changes

Refactoring

072defb

genlu enabled auto-merge March 18, 2022 22:17

sharwell reviewed Mar 21, 2022

View reviewed changes

pass the listener to the constructor

ebe077f

genlu merged commit f690017 into dotnet:main Mar 22, 2022

ghost added this to the Next milestone Mar 22, 2022

genlu deleted the UseCache branch March 22, 2022 00:03

allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022


		IDictionary<ProjectId, TProject> ProjectItemsCache { get; }

		CancellationToken DisposalToken { get; }

		var exportProvider = (IMefHostExportProvider)project.Solution.Workspace.Services.HostServices;
		var listenerProvider = exportProvider.GetExports<AsynchronousOperationListenerProvider>().Single().Value;

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu Mar 1, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu Mar 1, 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!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu commented Mar 16, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Mar 21, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

genlu commented Feb 25, 2022 •

edited

Loading

genlu Mar 1, 2022 •

edited

Loading

genlu Mar 1, 2022 •

edited

Loading

sharwell Mar 21, 2022 •

edited

Loading