Perf improvement for completion of unimported extension methods by genlu · Pull Request #45283 · dotnet/roslyn

genlu · 2020-06-18T06:01:07Z

I only did some quick measurement with stopwatch, the completion perf using repro project (ConsoleApp with reference to Accord.Math package) improved from ~65s to ms.

@CyrusNajmabadi could you please take a look?

FYI @AmadeusW

All array types for one elemnt type is consider same in our index. e.g. int[], int[][] and int[,] are just represent as int[]

…reeInfo

Since they are already from the same compilation that triggered completion.

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

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

CyrusNajmabadi · 2020-06-18T06:18:48Z

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

            => s_aliasMapListPool.Allocate();

+        protected static string CreateTargetTypeStringForArray(string elementTypeName)
+            => elementTypeName + "[]";


the + "[]" part is repeated several times. can we share code?

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

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

CyrusNajmabadi · 2020-06-18T06:23:50Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

-                    elementTypeSymbol = symbol.ElementType;
-                }
+                case INamedTypeSymbol namedType:
+                    return namedType.IsTupleType ? string.Empty : namedType.MetadataName;


do we have tests for these special cases?

This is actually incorrect. I have fixed the handling of ValueTuple type in source and added tests.

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

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

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

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

CyrusNajmabadi · 2020-06-18T06:28:20Z

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

            /// - By reference type of any types above
+            /// - Array types of with element of any types above
            /// </summary>
            public readonly bool IsComplexType;


if IsArrayType implies IsComplexType, should we use an enum instead?

No, array doesn't indicate it's complex type, it depends on the element of the array

ah... that wasn't totally clear to me from the docs (though they docs are correct). perhaps explicitly call out that simple array types (i.e. Int32[]) wouldn't be complex.

CyrusNajmabadi · 2020-06-18T06:30:31Z

ffrom ~65s to ms.

Awesome! What's the perf when called on an array type?

CyrusNajmabadi · 2020-06-18T06:32:24Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

+
+                    // We do not differentiate array of different kinds sicne they are all represented in the indices as "NonArrayElementTypeName[]"
+                    // e.g. int[], int[][], int[,], etc. are all represented as "int[]", whereas array of complex type such as T[] is "[]".
+                    return elementTypeName + "[]";


any concerns about things like (int, int)[]? or (int[], int[])? i.e. mixes of tuples/arrays?

if we use the "[]" elsewhere, consider a named ocnstant. or a hel that makes htis name.

any concerns about things like (int, int)[]? or (int[], int[])? i.e. mixes of tuples/arrays?

Those should work now.

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

CyrusNajmabadi · 2020-06-18T06:38:47Z

Overall this is looking fine to me. :) My overall request is to always be keeping an eye to simplicity and clarity. I think some small tweaks would help a lot. Thanks!

genlu · 2020-06-18T23:00:36Z

Awesome! What's the perf when called on an array type?

I don't have a concrete number, but it would be slower since it will have potentially a lot more symbols to check. But in the repro project I have it still takes less than a sec

CyrusNajmabadi · 2020-06-18T23:13:27Z

But in the repro project I have it still takes less than a sec

Awesome! That's still more than acceptable to get us past this major regression :)

…framework Set ignoreAssemblyKey to true during SymbolKey resolution

genlu · 2020-06-24T00:31:12Z

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

File #45404 to track this issue. Not fixing it as part of this PR.

The perf with ignoreAssemblyKey = true doesn't meet the bar

genlu · 2020-06-24T00:50:15Z

@CyrusNajmabadi Now the avg time for getting unimported extension method items (just computation in OOP, not including communication between devenv and OOP) is ~30ms in the repro solution, which takes 65s everything included for each completion session w/o this change. Could you please take another look?

CyrusNajmabadi · 2020-06-24T20:03:54Z

yup, i will look later today. ping me if i forget

...Features/Core/Portable/Completion/Providers/ImportCompletionProvider/ImportCompletionItem.cs

src/Workspaces/Core/Portable/FindSymbols/Extensions.cs

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

CyrusNajmabadi · 2020-06-24T20:55:03Z

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

+        public MultiDictionary<string, ExtensionMethodInfo>.ValueSet GetExtensionMethodInfoForReceiverType(string typeName)
+            => _receiverTypeNameToExtensionMethodMap != null
+                ? _receiverTypeNameToExtensionMethodMap[typeName]
+                : new MultiDictionary<string, ExtensionMethodInfo>.ValueSet(null, null);


singleton for this?

it's a struct

Good to know. a readonly static field might still be nice for clarity, but i don't care then :)

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

CyrusNajmabadi · 2020-06-24T21:06:54Z

done with pass.

…onProvider/ExtensionMethodImportCompletionHelper.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

...table/Completion/Providers/ImportCompletionProvider/ExtensionMethodImportCompletionHelper.cs

genlu · 2020-06-26T20:42:42Z

Thanks! @CyrusNajmabadi

genlu added 6 commits June 15, 2020 15:17

Make extention method of array types simple in index

b2b7139

All array types for one elemnt type is consider same in our index. e.g. int[], int[][] and int[,] are just represent as int[]

Make receiver type name part of the extension method filter

6878961

Add info about whether a complex receiver type is an array to Symbolt…

ca70f85

…reeInfo

Don't serialize method symbols from PE references

fda2b7d

Since they are already from the same compilation that triggered completion.

Add info for receiver of complex array type in syntax index

ca892c8

Fix completion of ext-method for receiver of type parameter type

386dd3b

genlu added the Area-IDE label Jun 18, 2020

genlu requested a review from a team as a code owner June 18, 2020 06:01

genlu requested a review from CyrusNajmabadi June 18, 2020 06:01