-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify MergedNamespaceDeclaration.MakeChildren to to allocate less #74698
Conversation
This method is showing up in a profile via the ArrayBuilder.ToDictionary calls. 1) Two dictionaries are created each call. The second Dictionary is only used for enumeration, and thus we can avoid it. 2) The ToDictionary implementation cannot use a PooledDictionary, whereas our invocation for types can. 3) The ToDictionary creates ArrayBuilders for each unique key. This is bad if there are a large number of unique keys (such as for our invocation for types). By creating such a large number of ArrayBuilders, we lose the benefit of the pooling, as the vast majority of the time we won't be able to return or get builders from the pool. I separated the processing of the namespaces and types collections into their own methods, as they have slightly different characteristics. Specifically: 1) The namespaces collection has few unique keys, but potentially large numbers of items in their value collections. 2) The types collection has many unique keys, and usually small numbers of items in their value collections. Handling namespaces is pretty straight forward using a Dictionary<string, ArrayBuilder<SingleNamespaceDeclaration>>. However, handling types required a bit less convention to get around the issue of creating a large number of ArrayBuilder instances. Instead, we use an object as the value, with it either being an ArrayBuilder<SingleTypeDeclaration> when an identity has been seen multiple times, or just a SingleTypeDeclaration when the identity has only been seen once. MergedNamespaceDeclaration.MakeChildren is showing up as about 1.5% of allocations in the typing section of the speedometer scrolling perf test profile. These changes should shave that down to about 0.5% or so.
Triggered an insertion PR to validate the perf benefits. Will update once it completes: Insertion PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/570768 MakeChildren isn't showing up in the profile against the run with these changes. |
src/Compilers/CSharp/Portable/Declarations/MergedNamespaceDeclaration.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Declarations/MergedNamespaceDeclaration.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Declarations/MergedNamespaceDeclaration.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Declarations/MergedNamespaceDeclaration.cs
Show resolved
Hide resolved
@dotnet/roslyn-compiler for 2nd review, speedometer results were agreeable |
@dotnet/roslyn-compiler for 2nd review. Thanks! |
This method is showing up in a profile via the ArrayBuilder.ToDictionary calls.
I separated the processing of the namespaces and types collections into their own methods, as they have slightly different characteristics. Specifically:
Handling namespaces is pretty straight forward using a Dictionary<string, ArrayBuilder>. However, handling types required a bit less convention to get around the issue of creating a large number of ArrayBuilder instances. Instead, we use an object as the value, with it either being an ArrayBuilder when an identity has been seen multiple times, or just a SingleTypeDeclaration when the identity has only been seen once.
MergedNamespaceDeclaration.MakeChildren is showing up as about 1.5% of allocations in the typing section of the speedometer scrolling perf test profile. These changes should shave that down to about 0.5% or so.
*** Relevant allocations from the typing section of the scrolling speedometer profile ***