Speed up progression searches by using the navigate-to index #54382

CyrusNajmabadi · 2021-06-25T07:16:39Z

The current progression search system works by literally walking the entire solution, producing compilations for all projects and symbols for every symbol therein. This is pretty terrible as it's enormously costly and uses a ton of ram. On my machine it takes a full 2 minutes to search Roslyn.sln and uses at least a GB of ram.

This PR introduces a new implementation of progression search that sits on top of the NavigateTo search index instead. This brings search time down <4 seconds, and uses around 900mb less memory. This also benefits from:

NavigateTo running OutOfProc
all the work we did to dedupe results in navto
the work to allow navto to return results before the solution (or oop) has fully loaded

However, this change is not a pure win. The way progression search works today is that all entries are backed by real symbols. This means that the presented nodes can expose commands (like 'find all callers'). These continue to work for normal progression items. However, this is not supported for tehse new search results (as they're not backed by a real symbol anymore).

The progression team is going to investigate though how often (if ever) these advanced commands are used on search results. We hypothesize that very few people will be affected by this, but we would like data to be sure. So, in the meantime, this will be off by default.

CyrusNajmabadi · 2021-06-25T07:17:14Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemDisplay.cs

-                // to.
-                spanStart = sourceText.Length;
-            }
+            var span = NavigateToUtilities.GetBoundedSpan(_searchResult.NavigableItem, sourceText);


extracted the above into a helper to use in progression as well.

CyrusNajmabadi · 2021-06-25T07:17:31Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemProvider.cs

-                return result;
-            }
-        }
+            => NavigateToUtilities.GetKindsProvided(_workspace.CurrentSolution);


similar. this moved to a common locatino so Progressin could use it.

src/VisualStudio/Core/Def/Implementation/Progression/ProgressionOptions.cs

Youssef1313 · 2021-06-25T08:25:19Z

src/VisualStudio/Core/Def/Implementation/Progression/GraphBuilder.cs

+        private static string GetIconString(Glyph glyph)
+        {
+            return glyph switch
+            {
+                Glyph.ClassPublic or Glyph.ClassProtected or Glyph.ClassPrivate or Glyph.ClassInternal => IconHelper.GetIconName("Class", GetAccessibility(glyph, Glyph.ClassPublic)),
+                Glyph.ConstantPublic or Glyph.ConstantProtected or Glyph.ConstantPrivate or Glyph.ConstantInternal => IconHelper.GetIconName("Field", GetAccessibility(glyph, Glyph.ConstantPublic)),
+                Glyph.DelegatePublic or Glyph.DelegateProtected or Glyph.DelegatePrivate or Glyph.DelegateInternal => IconHelper.GetIconName("Delegate", GetAccessibility(glyph, Glyph.DelegatePublic)),
+                Glyph.EnumPublic or Glyph.EnumProtected or Glyph.EnumPrivate or Glyph.EnumInternal => IconHelper.GetIconName("Enum", GetAccessibility(glyph, Glyph.EnumPublic)),
+                Glyph.EnumMemberPublic or Glyph.EnumMemberProtected or Glyph.EnumMemberPrivate or Glyph.EnumMemberInternal => IconHelper.GetIconName("EnumMember", GetAccessibility(glyph, Glyph.EnumMemberPublic)),
+                Glyph.ExtensionMethodPublic or Glyph.ExtensionMethodProtected or Glyph.ExtensionMethodPrivate or Glyph.ExtensionMethodInternal => IconHelper.GetIconName("Method", GetAccessibility(glyph, Glyph.ExtensionMethodPublic)),
+                Glyph.EventPublic or Glyph.EventProtected or Glyph.EventPrivate or Glyph.EventInternal => IconHelper.GetIconName("Event", GetAccessibility(glyph, Glyph.EventPublic)),
+                Glyph.FieldPublic or Glyph.FieldProtected or Glyph.FieldPrivate or Glyph.FieldInternal => IconHelper.GetIconName("Field", GetAccessibility(glyph, Glyph.FieldPublic)),
+                Glyph.InterfacePublic or Glyph.InterfaceProtected or Glyph.InterfacePrivate or Glyph.InterfaceInternal => IconHelper.GetIconName("Interface", GetAccessibility(glyph, Glyph.InterfacePublic)),
+                Glyph.MethodPublic or Glyph.MethodProtected or Glyph.MethodPrivate or Glyph.MethodInternal => IconHelper.GetIconName("Method", GetAccessibility(glyph, Glyph.MethodPublic)),
+                Glyph.ModulePublic or Glyph.ModuleProtected or Glyph.ModulePrivate or Glyph.ModuleInternal => IconHelper.GetIconName("Module", GetAccessibility(glyph, Glyph.ModulePublic)),
+                Glyph.PropertyPublic or Glyph.PropertyProtected or Glyph.PropertyPrivate or Glyph.PropertyInternal => IconHelper.GetIconName("Property", GetAccessibility(glyph, Glyph.PropertyPublic)),
+                Glyph.StructurePublic or Glyph.StructureProtected or Glyph.StructurePrivate or Glyph.StructureInternal => IconHelper.GetIconName("Structure", GetAccessibility(glyph, Glyph.StructurePublic)),
+                _ => null,
+            };


Anyway to instead reuse GetTags?

roslyn/src/Features/Core/Portable/Common/GlyphTags.cs

Line 14 in c1ec4b3

public static ImmutableArray<string> GetTags(Glyph glyph)

and also:

roslyn/src/Features/Core/Portable/Common/GlyphExtensions.cs

Line 211 in c1ec4b3

private static Accessibility GetAccessibility(ImmutableArray<string> tags)

yup. THat seems nice. Thanks!

ryzngard · 2021-06-25T19:42:11Z

src/Features/Core/Portable/Common/GlyphExtensions.cs

@@ -208,7 +208,7 @@ private static Glyph GetGlyph(string tag, ImmutableArray<string> allTags)
            return Glyph.None;
        }

-        private static Accessibility GetAccessibility(ImmutableArray<string> tags)
+        public static Accessibility GetAccessibility(ImmutableArray<string> tags)


Suggested change

public static Accessibility GetAccessibility(ImmutableArray<string> tags)

public static Accessibility GetAccessibility(this ImmutableArray<string> tags)

very nit here, but I would expect this to be an extension like the others in the file.

nooooooooooooooooooooooooooooooooooooo do not make highly specific extension methods on widely used types like this.

i'm not a fan of extending random collections. For example, you could have an ImmutableArray in many different contexts. Having this .GetAccessibility extension show up for all of those seems wonky to me :)

almost all static types called XXXExtensions though are extension methods. Should it move instead?

ryzngard · 2021-06-25T19:43:45Z

src/VisualStudio/Core/Def/Implementation/Progression/GraphBuilder.cs

+                NavigateToItemKind.Constant or
+                NavigateToItemKind.EnumItem or
+                NavigateToItemKind.Field => CodeNodeCategories.Field,


Suggested change

NavigateToItemKind.Constant or

NavigateToItemKind.EnumItem or

NavigateToItemKind.Field => CodeNodeCategories.Field,

NavigateToItemKind.Constant or

NavigateToItemKind.EnumItem or

NavigateToItemKind.Field => CodeNodeCategories.Field,

This is definitely a style preference, but I find it hard to know that items are linked to a single thing in switch expressions without the indentation.

ryzngard · 2021-06-25T19:45:11Z

src/VisualStudio/Core/Def/Implementation/Progression/GraphBuilder.cs

+            var groupName = glyph switch
+            {
+                Glyph.ClassPublic or Glyph.ClassProtected or Glyph.ClassPrivate or Glyph.ClassInternal => "Class",
+                Glyph.ConstantPublic or Glyph.ConstantProtected or Glyph.ConstantPrivate or Glyph.ConstantInternal => "Field",


Do we use the same glyph for constants and fields?

Yup! As per GetOrCreateNodeForFieldAsync it looks like we only distinguish fields and enum-members in progression.

src/VisualStudio/Core/Def/Implementation/Progression/ProgressionOptionsProvider.cs

CyrusNajmabadi added 8 commits June 24, 2021 14:06

Sure, why not

4a453c7

Fast progression search.

7cdaaad

Update tests

cdbede7

Fixup tests

d105390

Report as we get results

65cefe1

Multple matches

21dd1a3

Split tests

987a5c3

not roaming

c93d06a

CyrusNajmabadi requested review from davidwengier and dibarbet June 25, 2021 07:16

CyrusNajmabadi requested review from a team as code owners June 25, 2021 07:16

dotnet-issue-labeler bot added the Area-IDE label Jun 25, 2021

CyrusNajmabadi commented Jun 25, 2021

View reviewed changes

vert

380572b

Youssef1313 reviewed Jun 25, 2021

View reviewed changes

src/VisualStudio/Core/Def/Implementation/Progression/ProgressionOptions.cs Show resolved Hide resolved

Youssef1313 reviewed Jun 25, 2021

View reviewed changes

src/VisualStudio/Core/Def/Implementation/Progression/ProgressionOptions.cs Show resolved Hide resolved

Youssef1313 reviewed Jun 25, 2021

View reviewed changes

CyrusNajmabadi added 3 commits June 25, 2021 11:10

Simplify glyph computation

f684737

Remove unused property

f6ff0f7

Cleanup

c7a45ed

CyrusNajmabadi requested a review from ryzngard June 25, 2021 19:31

ryzngard approved these changes Jun 25, 2021

View reviewed changes

CyrusNajmabadi merged commit eb02555 into dotnet:main Jun 25, 2021

ghost added this to the Next milestone Jun 25, 2021

CyrusNajmabadi deleted the progression branch June 25, 2021 21:40

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up progression searches by using the navigate-to index #54382

Speed up progression searches by using the navigate-to index #54382

CyrusNajmabadi commented Jun 25, 2021

CyrusNajmabadi Jun 25, 2021

CyrusNajmabadi Jun 25, 2021

Youssef1313 Jun 25, 2021 •

edited

Loading

CyrusNajmabadi Jun 25, 2021

ryzngard Jun 25, 2021

sharwell Jun 25, 2021

CyrusNajmabadi Jun 25, 2021

ryzngard Jun 25, 2021

ryzngard Jun 25, 2021

ryzngard Jun 25, 2021

CyrusNajmabadi Jun 25, 2021

	public static Accessibility GetAccessibility(ImmutableArray<string> tags)
	public static Accessibility GetAccessibility(this ImmutableArray<string> tags)

Speed up progression searches by using the navigate-to index #54382

Speed up progression searches by using the navigate-to index #54382

Conversation

CyrusNajmabadi commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Jun 25, 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

Choose a reason for hiding this comment

Youssef1313 Jun 25, 2021 •

edited

Loading