Improve cross-project add-using results #63449

CyrusNajmabadi · 2022-08-17T16:52:56Z

FOllowup to #63438

Add-using supports adding references to source symbols located in sibling projects, and metadata-references from sibling projects. Traditionally, this was done by building a name-index for all those projects and references, and then having add-using search those indices to look for results.

This worked decently, but had a catch. It would only produce results if the index was fully up to date. So if you made changes in a project, you might not see any results from that project in other projects until we got around to reanalyzing that project.

This PR updates add-using to now allow using those indices even if they're stale. This means that if you are tryign to add a using for an type that already existed in a project which is stale, that will still work instead of you getting nothing.

Note: this is also safe in the case that the index contains information that is no longer applicable. That's because we just use the index to help find the info for a particular name to then lookup the symbol with that name in the project/reference. We still then try to find the symbol given that info. If we don't find it, that's fine and it just means we won't have an add-using option. It's a little extra CPU but should be generally quite rare. And it's a lot better to pay that tiny price occasionally if it allows us to use stale indices instead of waiting on them to be rebuilt.

CyrusNajmabadi · 2022-08-17T16:54:05Z

src/Features/Core/Portable/AddImport/SearchScopes/MetadataSymbolsSearchScope.cs

-                var service = _solution.Services.GetService<ISymbolTreeInfoCacheService>();
-                var info = await service.TryGetMetadataSymbolTreeInfoAsync(_solution, _metadataReference, CancellationToken).ConfigureAwait(false);
+                var service = _solution.Services.GetService<SymbolTreeInfoCacheService>();
+                var info = await service.TryGetPotentiallyStaleMetadataSymbolTreeInfoAsync(_solution, _metadataReference, CancellationToken).ConfigureAwait(false);


here we now allow for a stale index to come back. if we get no index at all, that's fine and we gracefully handle it. If we get a stale index, that's also fine as FindAsync then simply uses the index data to try to find matching symbols in _assembly. It is completely ok with that 'Find' step not finding anything and still returning no results.

CyrusNajmabadi · 2022-08-17T16:55:09Z

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

@@ -86,7 +86,7 @@ public static async ValueTask UpdateCacheAsync(Project project, CancellationToke
                    await GetUpToDateCacheEntryAsync(relevantProject, cacheService, cancellationToken).ConfigureAwait(false);

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


the loadOnly flag entirely went away. There are now just two distinct types of callers. Those that want the completely up-to-date index (and are willing to compute it), and those that are are ok with a stale index and will either use what is already there, or will try to load whatever index we last stored to disk if not.

CyrusNajmabadi · 2022-08-17T16:58:14Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs

+    /// necessary.  The secondary purpose is to generate a minimal set of symbols when there is a match, though that
+    /// will still incur a heavy cost (for example, getting the <see cref="IAssemblySymbol"/> root symbol for a
+    /// particular project).
+    /// </summary>


i figured it was finally time to have a comment for this :)

CyrusNajmabadi · 2022-08-17T16:59:34Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs

-        /// created.
-        /// </summary>
-        private readonly Task<SpellChecker> _spellCheckerTask;
+        private readonly SpellChecker _spellChecker;


this used to be lazy when everything was in-proc and memory/cpu was very tight. Now that this is all OOP, it's just much simpler to have no lazyness in the internal data of this index. measuring things in the benchmarks showed no issue moving this to not be delayed. Also, since this is used by add-using, it's highly likely for this to be needed almost immediately as the add-using codepath runs for tons of different user error scenarios.

CyrusNajmabadi · 2022-08-17T17:10:34Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo_Metadata.cs

@@ -126,7 +116,6 @@ private static string GetMetadataNameWithoutBackticks(MetadataReader reader, Str
            SolutionServices services,
            SolutionKey solutionKey,
            PortableExecutableReference reference,
-            Checksum checksum,


no need to pass checksums along. they are already cached in dictionaries that we have access to so it can just be retrieved when necessary.

CyrusNajmabadi · 2022-08-23T03:45:44Z

src/Features/Core/Portable/AddImport/SearchScopes/MetadataSymbolsSearchScope.cs

@@ -50,15 +50,12 @@ public override SymbolReference CreateReference<T>(SymbolResult<T> searchResult)
                SymbolFilter filter, SearchQuery searchQuery)
            {
                var service = _solution.Services.GetService<SymbolTreeInfoCacheService>();
-                var info = await service.TryGetMetadataSymbolTreeInfoAsync(_solution, _metadataReference, CancellationToken).ConfigureAwait(false);
+                var info = await service.TryGetPotentiallyStaleMetadataSymbolTreeInfoAsync(_solution, _metadataReference, CancellationToken).ConfigureAwait(false);


renamed to make the staleness possibility obvious.

Is PotentiallyStale more clear than Cached? Caches always can be stale :)

i would be ok with that :)

CyrusNajmabadi · 2022-08-23T03:46:56Z

src/Workspaces/Core/Portable/FindSymbols/Declarations/DeclarationFinder.cs


-                    // We must have an index since we passed in loadOnly: false here.
                    Contract.ThrowIfNull(info);


probably not necessary (we have NRT on these apis). but it doesn't hurt to still have this check.

CyrusNajmabadi · 2022-08-23T14:54:48Z

@dibarbet @ryzngard ptal. Thanks!

ryzngard · 2022-08-23T20:42:22Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs

-            return Task.FromResult(new SpellChecker(
-                checksum, sortedNodes.Select(n => n.Name.AsMemory())));
-        }
+        private static SpellChecker CreateSpellChecker(Checksum checksum, ImmutableArray<Node> sortedNodes)


is this really needed anymore?

this is actually called by a few places :) so i'dlike to leave in.

ryzngard · 2022-08-23T20:43:13Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfoCacheService.cs

-            if (_metadataIdToInfo.TryGetValue(metadataId, out var metadataInfo) &&
-                metadataInfo.SymbolTreeInfo.Checksum == checksum)
-            {
+            // See if the last value produced exactly matches what the caller is asking for.  If so, return that.


This comment is wrong now isn't it? It just checks if we have any cached value

ryzngard · 2022-08-23T20:44:15Z

src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfoCacheService.cs

-            if (_projectIdToInfo.TryGetValue(project.Id, out var projectInfo) &&
-                projectInfo.Checksum == checksum)
-            {
+            // See if the last value produced exactly matches what the caller is asking for.  If so, return that.


Same as https://github.com/dotnet/roslyn/pull/63449/files#r953091508

can fix in followup PR :)

ryzngard

love all the added comments. Overall this looks correct and easier to understand

CyrusNajmabadi · 2022-08-23T21:14:35Z

love all the added comments. Overall this looks correct and easier to understand

Thanks. Will make a followup for the naming/comment issues.

CyrusNajmabadi added 9 commits August 17, 2022 08:07

IN progress

bed2fc9

Add docs

e51c74f

Use values in the cache if present

1b1d06f

In progres

2e8d7d4

In progress

5e9d209

IN progress

383397f

Allow stale values to be used in add-import

44802dc

Allow stale values to be used in add-import

4293be7

Merge branch 'removeIncremental3' into betterAddUsing

0ace3f5

dotnet-issue-labeler bot added the Area-IDE label Aug 17, 2022

CyrusNajmabadi commented Aug 17, 2022

View reviewed changes

CyrusNajmabadi added 3 commits August 17, 2022 09:56

remove comment

1a18806

Add doc

f84fae8

Merge branch 'removeIncremental3' into betterAddUsing

f373779

CyrusNajmabadi commented Aug 17, 2022

View reviewed changes

CyrusNajmabadi added 7 commits August 17, 2022 10:16

Simplify

617afdf

Merge branch 'removeIncremental3' into betterAddUsing

e750917

Merge branch 'removeArg' into betterAddUsing

f626483

Merge branch 'removeIncremental3' into betterAddUsing

b56e7b3

Merge branch 'removeIncremental3' into betterAddUsing

fd8ee15

Merge branch 'removeIncremental3' into betterAddUsing

ef461b7

Merge branch 'main' into betterAddUsing

00c4854

CyrusNajmabadi marked this pull request as ready for review August 23, 2022 03:44

CyrusNajmabadi requested a review from a team as a code owner August 23, 2022 03:44

CyrusNajmabadi commented Aug 23, 2022

View reviewed changes

REmove named parameter

2967979

CyrusNajmabadi added 2 commits August 22, 2022 20:55

Add comments and rename methods

1df19e6

Rename

2e6e04e

CyrusNajmabadi requested review from dibarbet and ryzngard August 23, 2022 04:04

CyrusNajmabadi added 2 commits August 22, 2022 21:32

Simplify

099d4e0

Simplify

4ed3696

ryzngard reviewed Aug 23, 2022

View reviewed changes

ryzngard approved these changes Aug 23, 2022

View reviewed changes

CyrusNajmabadi merged commit a128904 into dotnet:main Aug 23, 2022

CyrusNajmabadi deleted the betterAddUsing branch August 23, 2022 21:14

ghost added this to the Next milestone Aug 23, 2022

dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022

CyrusNajmabadi mentioned this pull request Oct 14, 2022

Cache indices against our PEReference object, not the underlying metadata ID #64742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cross-project add-using results #63449

Improve cross-project add-using results #63449

CyrusNajmabadi commented Aug 17, 2022

CyrusNajmabadi Aug 17, 2022

CyrusNajmabadi Aug 17, 2022

CyrusNajmabadi Aug 17, 2022

CyrusNajmabadi Aug 17, 2022

CyrusNajmabadi Aug 17, 2022

CyrusNajmabadi Aug 23, 2022

ryzngard Aug 23, 2022

CyrusNajmabadi Aug 23, 2022

CyrusNajmabadi Aug 23, 2022

CyrusNajmabadi commented Aug 23, 2022

ryzngard Aug 23, 2022

CyrusNajmabadi Aug 23, 2022

ryzngard Aug 23, 2022

CyrusNajmabadi Aug 23, 2022

ryzngard Aug 23, 2022

CyrusNajmabadi Aug 23, 2022

ryzngard left a comment

CyrusNajmabadi commented Aug 23, 2022


		// We must have an index since we passed in loadOnly: false here.
		Contract.ThrowIfNull(info);

Improve cross-project add-using results #63449

Improve cross-project add-using results #63449

Conversation

CyrusNajmabadi commented Aug 17, 2022

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

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 23, 2022

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

ryzngard left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 23, 2022