Improve searching for symbols when a name is known ahead of time. (Including Compilation.GetEntryPoint()) by CyrusNajmabadi · Pull Request #26325 · dotnet/roslyn

CyrusNajmabadi · 2018-04-22T19:08:48Z

Found as part of this review: #26316

@jcouv Would like to call compilation.GetEntryPoint inside an analyzer. However, this is currently very expensive as it will realize all namespace, type and member symbols in a compilation. We can improve things greatly here through use of the decl table. We only need to analyze types that actually contain a 'Main' member in them, vastly decreasing the amount of work and garbage that is necessary.

I've also decreased the cost of decl table searches. Avoiding linq, and other unnecessary allocations when we hit each decl.

CyrusNajmabadi · 2018-04-22T19:09:22Z

Tagging @jcouv @dotnet/roslyn-compiler . #Resolved

CyrusNajmabadi · 2018-04-22T19:14:30Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

+        /// <summary>
+        /// Return source declaration symbols whose name matches the provided name
+        /// </summary>
+        internal abstract IEnumerable<ISymbol> GetSymbolsWithName(string name, SymbolFilter filter = SymbolFilter.TypeAndMember, CancellationToken cancellationToken = default(CancellationToken));


i woudl like to make this public. This would also be a valuable member for the FindReferencesEngine to have. Currently we have to call into the member above this which is quite expensive.

I will do that in another PR though.

CyrusNajmabadi · 2018-04-22T19:18:25Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

        }

-        private class SymbolSearcher
+        private abstract class AbstractSymbolSearcher


note: i would like to move these to sibling files. However, i didn't want to interfere with teh diff, so i kept them in this file for now. I'll move if that's what people would like, once this is reviewed. #Resolved

I recommend doing the move in a separate iteration of this PR.

In reply to: 183247833 [](ancestors = 183247833)

Sounds good. #Resolved

CyrusNajmabadi · 2018-04-22T20:24:37Z

src/Compilers/CSharp/Portable/Compiler/EntryPointCandidateFinder.cs

-                                {
-                                    continue;
-                                }
-                            }


this moved into MethodSymbol.IsEntryPointCandidate #Resolved

CyrusNajmabadi · 2018-04-22T21:02:00Z

src/Compilers/VisualBasic/Portable/Compilation/EntryPointCandidateFinder.vb

                        Dim method = DirectCast(member, MethodSymbol)
-                        ' check if this is an entry point
-                        If Not method.IsSubmissionConstructor AndAlso _entryPointCandidates IsNot Nothing AndAlso Not method.IsImplicitlyDeclared AndAlso method.IsEntryPointCandidate Then
+                        If _entryPointCandidates IsNot Nothing AndAlso method.IsEntryPointCandidate Then


the other checks moved into IsEntryPointCandidate. #Resolved

CyrusNajmabadi · 2018-04-22T21:06:31Z

src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb

+                            Return True
+                        End If
+                    Next
+                Next


note: it's a pity we can't just do typeDecl.MemberNames.Contains(_name) However, MemberNames is just an array, and not a set with an appropriate comparer.

This means hte algorithm is O(n) with the number of member names to check. If we can actually use a set here, then we are only O(n) with the number of types to check. There are vastly less types than members, so that would be a substantive improvement.

We could potentially replace that array with a SmallDictionary.Keys, but that could potentially have larger remifications. Personally, i don't htink it would be an issue, as this would be just one small change per TypeDeclaration, but it seemed out of scope of this PR. @VSadov for his thoughts here.

Note: i was the original person who added MemberNames to the decl table. It was always intended, from the beginning, to be a fast way to check for existence of a member and/or to get that member quickly. I believe i started things out with a set, but it's so lost in the annals of time for me that i'm not sure. However, it does feel like this would be an easy tweak to make, with minimal issues, for the beneficial goal of speeding up these queries. Being able to just do simple O(1) lookup per type, instead of N checks would be very nice.

gafter · 2018-04-23T20:29:09Z

Very nice. LGTM #Resolved

gafter

CyrusNajmabadi · 2018-04-23T20:41:25Z

Is this used anywhere? If not please delete it.

@gafter Unfortunately yes. It's used when the user supplies a class name, and is what finds the right method inside that class. That said, that's soooo narrow at this point, that it could be removed with the client just enumerating the methods of the class for an appropriate candidate. If you'd like, i can do that. Let me know. #Resolved

CyrusNajmabadi · 2018-04-23T21:50:32Z

@jcouv Can you take a look? #Resolved

CyrusNajmabadi · 2018-04-23T22:13:05Z

@gafter i removed the types as they served practically no purpose anymore. #Resolved

CyrusNajmabadi · 2018-04-24T18:06:33Z

@jcouv Thoughts? #Resolved

jcouv · 2018-04-24T18:08:36Z

Looking now #Resolved

jcouv · 2018-04-24T19:05:52Z

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

+
+            protected override bool ShouldCheckTypeForMembers(MergedTypeDeclaration current)
+            {
+                foreach (var typeDecl in current.Declarations)


Could you type this var. Thanks #Closed

Sure! #resolved #Resolved

jcouv · 2018-04-24T19:09:11Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

-        }
+            get
+            {
+                if (this.IsPartialDefinition() &&


Is this already covered by test? #Closed

Yes: ERR_NoEntryPoint_PartialClass (and siblings). #resolved #Resolved

jcouv · 2018-04-24T19:11:06Z

src/Compilers/CSharp/Test/Symbol/Compilation/SymbolSearchTests.cs

+            Assert.Throws<ArgumentException>(() =>
+            {
+                var compilation = GetTestCompilation();
+                compilation.GetSymbolsWithName("", SymbolFilter.None);


GetSymbolsWithName [](start = 28, length = 18)

I expect a similar case for ContainsSymbolsWithName (passing in a name, but SymbolFilter.None) #Closed

will do. #Closed

Done. #resolved #Closed

jcouv · 2018-04-24T19:11:51Z

src/Compilers/CSharp/Test/Symbol/Compilation/SymbolSearchTests.cs

+        private static void Test(CSharpCompilation compilation, string name, bool includeNamespace, bool includeType, bool includeMember, int count)
+        {
+            var filter = SymbolFilter.None;
+            filter = includeNamespace ? filter | SymbolFilter.Namespace : filter;


Consider parens for readability #Closed

Done. #resolved.

jcouv · 2018-04-24T19:15:01Z

src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb

+        End Function
+
+        ''' <summary>
+        ''' Return true if there Is a source declaration symbol name that matches the provided name.


Is [](start = 33, length = 2)

Typo: Is from copy&paste (in a couple of places) #Closed

#fixed. thanks! #Resolved

jcouv · 2018-04-24T19:20:05Z

src/Compilers/VisualBasic/Portable/Symbols/MethodSymbol.vb

        ''' <returns>True if the method can be used as an entry point.</returns>
        Friend ReadOnly Property IsEntryPointCandidate As Boolean
            Get
+                If Me.ContainingType.IsEmbedded Then


Were those bugs? Do we have tests to cover? #Closed

This was just code that was in the EntryPointWalker. I just moved it into this method so that the logic for entrypoitn consideration was localized into one single location. #Closed

jcouv · 2018-04-24T19:20:25Z

src/Compilers/VisualBasic/Test/Semantic/Compilation/SymbolSearchTests.vb

+
+            Assert.Throws(Of ArgumentException)(Sub()
+                                                    Dim compilation = GetTestCompilation()
+                                                    compilation.GetSymbolsWithName("", SymbolFilter.None)


Same here (test ContainsSymbolsWithName with None) #Closed

sure #Closed

jcouv

Done with review pass (iteration 22).
Feels like some tests may be missing relative to change in entry point detection logic.

jcouv

LGTM Thanks!

jcouv · 2018-04-24T19:32:30Z

@jaredpar for approval to merge for 15.8. Thanks

CyrusNajmabadi · 2018-04-24T19:39:33Z

retest ubuntu_16_debug_prtest please

jaredpar · 2018-04-24T21:27:11Z

approved

…names searches. (#26331) Followup to #26325 and #26330. This PR updates the IDE to forward certain helpers to these more efficient implementations. This helps things out by more quickly being able to determine if a type even contains a member with name, and thus whether or not it should even be hydrated into a symbol and have its members created. Previous we would have to do a linear scan on all the members in a type to determine this. Now this data is in a set which can be queried much more efficiently.

CyrusNajmabadi requested a review from a team as a code owner April 22, 2018 19:08

jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Apr 22, 2018

jcouv added this to the 15.8 milestone Apr 22, 2018

Improve searching for symbols when a name is known ahead of time.

941b6ce

CyrusNajmabadi force-pushed the symbolSearchByName branch from 9bb9106 to 941b6ce Compare April 22, 2018 19:10

Simplify.

c4e637b

CyrusNajmabadi commented Apr 22, 2018

View reviewed changes

CyrusNajmabadi mentioned this pull request Apr 22, 2018

MakeMethodAsync: don't rename entry point methods #26316

Merged

CyrusNajmabadi changed the title ~~Improve searching for symbols when a name is known ahead of time.~~ Improve searching for symbols when a name is known ahead of time. (Including Compilation.GetEntryPoint()) Apr 22, 2018

CyrusNajmabadi commented Apr 22, 2018

View reviewed changes

CyrusNajmabadi added 4 commits April 22, 2018 12:42

Add VB impl.

def0e3a

Make VB 'GetEntryPoint' sit on top of GetSymbolsWithName

fa70f51

VB tests.

d9bb572

C# tests.

a6628b5

CyrusNajmabadi commented Apr 22, 2018

View reviewed changes

CyrusNajmabadi added 6 commits April 22, 2018 13:39

Add faster 'ContainsName' helpers as well.

feb0866

Add tests.

acb5d0b

Fixup VB (which doesn't use a case insensitive hashset for these names.

b15b15e

revert hts file.

ba83b19

Add comment.

03a5775

Add comment.

e411869

CyrusNajmabadi commented Apr 22, 2018

View reviewed changes

Simplify pooling.

bc27204

CyrusNajmabadi commented Apr 22, 2018

View reviewed changes

This was referenced Apr 23, 2018

Symbol API exposes data that can be mutated by the caller #26328

Closed

Improve searching for symbols when a name is known ahead of time. (And avoid O(n) enumerations) #26330

Merged

Fixup tests.

d8b3e03

CyrusNajmabadi mentioned this pull request Apr 23, 2018

Use new compiler symbol search helpers that are optimized for string names searches. #26331

Merged

gafter approved these changes Apr 23, 2018

View reviewed changes

Remove the old finders.

a4fe4d4

jcouv reviewed Apr 24, 2018

View reviewed changes

CyrusNajmabadi added 2 commits April 24, 2018 12:07

Use explicit type name.

f874b9b

Merge branch 'master' into symbolSearchByName

4fbb412

jcouv reviewed Apr 24, 2018

View reviewed changes

Add parens for readability.

5afd859

jcouv reviewed Apr 24, 2018

View reviewed changes

Fix spelling.

0456ea2

jcouv reviewed Apr 24, 2018

View reviewed changes

Add test.

3f2163a

jcouv approved these changes Apr 24, 2018

View reviewed changes

jcouv self-assigned this Apr 24, 2018

jaredpar added the Approved to merge label Apr 24, 2018

jcouv merged commit 93b8412 into dotnet:master Apr 24, 2018

CyrusNajmabadi deleted the symbolSearchByName branch April 24, 2018 22:10

Conversation

CyrusNajmabadi commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 23, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018 • 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!

CyrusNajmabadi Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 22, 2018

Choose a reason for hiding this comment

Uh oh!

gafter commented Apr 23, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 23, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 23, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 23, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 24, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Apr 24, 2018 • 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!

CyrusNajmabadi Apr 24, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 24, 2018 • 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!

CyrusNajmabadi Apr 24, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

CyrusNajmabadi commented Apr 22, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 22, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 22, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 23, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 22, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 22, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 22, 2018 •

edited

Loading

gafter commented Apr 23, 2018 •

edited by jcouv

Loading

CyrusNajmabadi commented Apr 23, 2018 •

edited by jcouv

Loading

CyrusNajmabadi commented Apr 23, 2018 •

edited by jcouv

Loading

CyrusNajmabadi commented Apr 23, 2018 •

edited by jcouv

Loading

CyrusNajmabadi commented Apr 24, 2018 •

edited by jcouv

Loading

jcouv commented Apr 24, 2018 •

edited

Loading

jcouv Apr 24, 2018 •

edited

Loading

CyrusNajmabadi Apr 24, 2018 •

edited by jcouv

Loading

jcouv Apr 24, 2018 •

edited

Loading

CyrusNajmabadi Apr 24, 2018 •

edited by jcouv

Loading

jcouv Apr 24, 2018 •

edited

Loading

CyrusNajmabadi Apr 24, 2018 •

edited by jcouv

Loading

CyrusNajmabadi Apr 24, 2018 •

edited by jcouv

Loading

jcouv Apr 24, 2018 •

edited

Loading

jcouv Apr 24, 2018 •

edited

Loading

CyrusNajmabadi Apr 24, 2018 •

edited by jcouv

Loading

jcouv Apr 24, 2018 •

edited

Loading

CyrusNajmabadi Apr 24, 2018 •

edited by jcouv

Loading

jcouv Apr 24, 2018 •

edited

Loading

CyrusNajmabadi Apr 24, 2018 •

edited by jcouv

Loading