Share more data in the SyntaxTreeIndex by CyrusNajmabadi · Pull Request #54057 · dotnet/roslyn

CyrusNajmabadi · 2021-06-12T04:18:56Z

I recommend reviewing this one commit at a time. The two main changes are:

instead of descending into all nodes in a tree and seeing which can become DeclaredSymbolInfo, we do an explicit loop over the top level items in a file that produce these.
as we descend into the top level items we compute and cache once strings that will be used by all children. this saves a lot of wasted duplicate string allocations.

CyrusNajmabadi · 2021-06-12T04:33:00Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs

            }
        }

-        private static void AddExtensionMethodInfo(


this moved into the IDeclaredSYmbolInfoService

CyrusNajmabadi · 2021-06-12T04:34:13Z

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs

+        where TMemberDeclarationSyntax : SyntaxNode
    {
-        private const string GenericTypeNameManglingString = "`";
-        private static readonly string[] s_aritySuffixesOneToNine = { "`1", "`2", "`3", "`4", "`5", "`6", "`7", "`8", "`9" };


these moved to another (non-generic) helper type.

CyrusNajmabadi · 2021-06-12T05:07:46Z

src/EditorFeatures/VisualBasicTest/NavigateTo/NavigateToTests.vb

+                Dim item = (Await _aggregator.GetItemsAsync("M")).Single
+                VerifyNavigateToResultItem(item, "M", "[|M|]()", PatternMatchKind.Exact, NavigateToItemKind.Method, Glyph.MethodPublic, additionalInfo:=String.Format(FeaturesResources.in_0_project_1, "A", "Test"))
+            End Function)
+        End Function


abstract methods never worked in VB navigate to. who knew.

CyrusNajmabadi · 2021-06-12T05:08:06Z

src/Workspaces/CSharp/Portable/FindSymbols/CSharpDeclaredSymbolInfoFactoryService.cs

-                        GetContainerDisplayName(node.Parent),
-                        GetFullyQualifiedContainerName(node.Parent),
+                        containerDisplayName,
+                        fullyQualifiedContainerName,


these values are computed once per container and passed in.

CyrusNajmabadi · 2021-06-12T05:09:18Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex.cs

            CancellationToken cancellationToken)
        {
+            if (!document.SupportsSyntaxTree)
+                return null;


adding these checks simplified CreateIndexAsync later on.

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs

…Index_Create.cs

into declaredInfos

CyrusNajmabadi · 2021-06-13T22:12:47Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs

-        bool TryGetAliasesFromUsingDirective(SyntaxNode node, out ImmutableArray<(string aliasName, string name)> aliases);
-
-        string GetRootNamespace(CompilationOptions compilationOptions);
+        void AddDeclaredSymbolInfos(Document document, SyntaxNode root, ArrayBuilder<DeclaredSymbolInfo> declaredSymbolInfos, Dictionary<string, ArrayBuilder<int>> extensionMethodInfo, CancellationToken cancellationToken);


all the above methods still exist, but are only used int he impl of AddDeclaredSymbolInfos, so they dont' need be exposed externally.

CyrusNajmabadi · 2021-06-13T22:13:42Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs

-            var stringTable = GetStringTable(project);
+            Contract.ThrowIfFalse(document.SupportsSyntaxTree);
+
+            var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);


pulled this await up, and put everything else in a synchronous method to prevent pooled collections from being used across awaits (As per @sharwell ).

CyrusNajmabadi · 2021-06-13T22:15:05Z

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs

                            containsGlobalAttributes = containsGlobalAttributes || syntaxFacts.IsGlobalAttribute(node);
                            containsConversion = containsConversion || syntaxFacts.IsConversionExpression(node);
-
-                            if (syntaxFacts.IsUsingAliasDirective(node) && infoFactory.TryGetAliasesFromUsingDirective(node, out var aliases))


aliases were used only inside the 'DeclaredSymbolInfo' computation. so all this moved into the other type.

CyrusNajmabadi · 2021-06-13T22:15:58Z

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs

+        protected abstract SyntaxList<TMemberDeclarationSyntax> GetChildren(TCompilationUnitSyntax node);
+        protected abstract SyntaxList<TMemberDeclarationSyntax> GetChildren(TNamespaceDeclarationSyntax node);
+        protected abstract SyntaxList<TMemberDeclarationSyntax> GetChildren(TTypeDeclarationSyntax node);
+        protected abstract IEnumerable<TMemberDeclarationSyntax> GetChildren(TEnumDeclarationSyntax node);


enums are a SeparatedSyntaxList in C#, and a SyntaxList in VB. so we need an IEnumerable here.

i considered making this generic and adding TEnumMemberDeclarationListSyntax ... where TEnumMemberDeclarationListSyntax : IEnumerable<TMemberDeclarationSyntax>. However, that doesn't help as foreaching will still allocate the IEnumerator. We'd need to have abstracts for the count/indexing to avoid any allocations here, and it just seemed to overwrought to be worth it.

CyrusNajmabadi · 2021-06-13T22:17:31Z

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs

+                {
+                    if (this.TryGetAliasesFromUsingDirective(usingAlias, out var current))
+                        AddAliases(aliases, current);
+                }


this logic is incorrect (but was incorrect originally as well). If you have multiple nested namespaces (possibly as symbols) one namespace's aliases with bleed into the others. HOwever, this is probably very rare in practice (certainly no one has reported an issue there), so it hasn't been an issue.

CyrusNajmabadi · 2021-06-13T22:17:40Z

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs

+                cancellationToken);
+        }
+
+        protected void AddExtensionMethodInfo(


this moved unchanged.

CyrusNajmabadi · 2021-06-13T22:17:47Z

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs

+            arrayBuilder.Add(declaredSymbolInfoIndex);
+        }
+
+        private static void AddAliases(Dictionary<string, string> allAliases, ImmutableArray<(string aliasName, string name)> aliases)


this moved unchanged.

CyrusNajmabadi · 2021-06-13T22:18:02Z

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs

-            Debug.Assert(arity > 0);
-            return (arity <= s_aritySuffixesOneToNine.Length)
-                ? s_aritySuffixesOneToNine[arity - 1]
-                : string.Concat(GenericTypeNameManglingString, arity.ToString(CultureInfo.InvariantCulture));


this moved into ArityUtilities

CyrusNajmabadi · 2021-06-13T22:19:06Z

src/Workspaces/CSharp/Portable/FindSymbols/CSharpDeclaredSymbolInfoFactoryService.cs

+            Dictionary<string, string> aliases,
+            Dictionary<string, ArrayBuilder<int>> extensionMethodInfo,
+            string containerDisplayName,
+            string fullyQualifiedContainerName,


a core change is that these two strings are now computed by teh caller and passed into all children, saving 5% of allocs in NavTo as reported by @sharwell .

CyrusNajmabadi · 2021-06-13T22:19:31Z

src/Workspaces/CSharp/Portable/FindSymbols/CSharpDeclaredSymbolInfoFactoryService.cs


-        public override bool TryGetDeclaredSymbolInfo(StringTable stringTable, SyntaxNode node, string rootNamespace, out DeclaredSymbolInfo declaredSymbolInfo)
+        protected override void AddDeclaredSymbolInfosWorker(
+            SyntaxNode container,


by passing in the container, we save a lot of ugly computation in the VB side trying to figure out waht the container is for any particular member/type.

CyrusNajmabadi · 2021-06-13T22:20:43Z

src/Workspaces/CSharp/Portable/FindSymbols/CSharpDeclaredSymbolInfoFactoryService.cs

+                case SyntaxKind.FieldDeclaration:
+                case SyntaxKind.EventFieldDeclaration:
+                    var fieldDeclaration = (BaseFieldDeclarationSyntax)node;
+                    foreach (var variableDeclarator in fieldDeclaration.Declaration.Variables)


this logic got much simpler as we don't have to hit every vardecl in the file (which might be everywhere) and then filter to the subset that are fields/events. instead we know if we've hit a field/event and can just process its vardecls directly.

CyrusNajmabadi · 2021-06-13T22:22:18Z

src/Workspaces/VisualBasic/Portable/FindSymbols/VisualBasicDeclaredSymbolInfoFactoryService.vb

+                node = DirectCast(node, EventBlockSyntax).EventStatement
+            ElseIf TypeOf node Is MethodBlockBaseSyntax Then
+                node = DirectCast(node, MethodBlockBaseSyntax).BlockStatement
            End If


the additional of MethodBlockBaseSyntax here is new. We effectively did not support abstract VB method before in navto. i fixed that as part of this change.

CyrusNajmabadi · 2021-06-13T22:22:36Z

src/Workspaces/VisualBasic/Portable/FindSymbols/VisualBasicDeclaredSymbolInfoFactoryService.vb

                        If(kind = SyntaxKind.ModuleBlock, DeclaredSymbolInfoKind.Module,
                        DeclaredSymbolInfoKind.Struct))),
-                        GetAccessibility(typeBlock, typeBlock.BlockStatement.Modifiers),
+                        GetAccessibility(container, typeBlock, typeBlock.BlockStatement.Modifiers),


passing the container in greatly simplified accessibility logic in several of these blocks.

CyrusNajmabadi · 2021-06-13T22:23:13Z

src/Workspaces/VisualBasic/Portable/FindSymbols/VisualBasicDeclaredSymbolInfoFactoryService.vb

+        End Function
+
+        Protected Overrides Function GetUsingAliases(node As NamespaceBlockSyntax) As SyntaxList(Of ImportsStatementSyntax)
+            Return Nothing


will doc that VB does not have imports inside a namespace.

CyrusNajmabadi added 5 commits June 11, 2021 20:41

Break out dedicated loop for creating index info

14f74bc

VBimpl

99b008d

Building

3a57787

Add extension method info

6c0b84f

Compute common string info once.

7afc371

CyrusNajmabadi requested review from genlu and sharwell June 12, 2021 04:18

CyrusNajmabadi requested a review from a team as a code owner June 12, 2021 04:18

ghost added the Area-IDE label Jun 12, 2021

CyrusNajmabadi added 2 commits June 11, 2021 21:22

Do not pass rootnamespace along

18b9082

Simplify and separate out sync/async

15e83f0

CyrusNajmabadi commented Jun 12, 2021

View reviewed changes

CyrusNajmabadi added 2 commits June 11, 2021 21:55

Add support for abstract methods

37a38a8

Simplify accessibility computation

b347d23

CyrusNajmabadi commented Jun 12, 2021

View reviewed changes

move alias processing into service

cd78a0b

CyrusNajmabadi commented Jun 12, 2021

View reviewed changes

src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTreeIndex_Create.cs Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 5 commits June 11, 2021 22:24

Update src/Workspaces/Core/Portable/FindSymbols/SyntaxTree/SyntaxTree…

e240232

…Index_Create.cs

Fix rename

cda9c99

Fix vb

fab017c

Merge branch 'declaredInfos' of https://github.com/CyrusNajmabadi/roslyn

9714b48

into declaredInfos

Fix parenting

57c7964

CyrusNajmabadi requested a review from davidwengier June 13, 2021 22:10

CyrusNajmabadi commented Jun 13, 2021

View reviewed changes

davidwengier approved these changes Jun 14, 2021

View reviewed changes

CyrusNajmabadi merged commit a34b3be into dotnet:main Jun 15, 2021

ghost added this to the Next milestone Jun 15, 2021

CyrusNajmabadi deleted the declaredInfos branch June 15, 2021 03:21

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

Conversation

CyrusNajmabadi commented Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jun 12, 2021 • 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CyrusNajmabadi commented Jun 12, 2021 •

edited

Loading

CyrusNajmabadi Jun 12, 2021 •

edited

Loading