Reduce allocations in AssemblySymbol.GetTopLevelTypeByMetadataName #79810

ToddGrun · 2025-08-06T15:08:05Z

This allocation shows up as a varying amount of allocations (from 0.1% to 10%) in the Roslyn CodeAnalysis process during completion in the CompletionInCohosting razor speedometer test.

The issue here is that the size of this array can exceed the the PooledArrayLengthLimitExclusive threshold specified in ArrayBuilder, thus causing items to not get placed back into the standard pool.

Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/659384

Profiles from the test insertion baseline exhibited large variations in AssemblySymbol[] allocations. Some had 15 MB allocated of this type, some had over a GB allocated. With this change, it was 1-2 MB.

*** From one of the bad before profiles ***

*** None of the after profiles had high allocation numbers for AssemblySymbol[], as evidenced by ***

This allocation shows up as over 5% of allocations during completion in the CompletionInCohosting razor speedometer test. The issue here is that the size of this array can exceed the the PooledArrayLengthLimitExclusive threshold specified in ArrayBuilder, thus causing items to not get placed back into the standard pool.

CyrusNajmabadi · 2025-08-06T15:44:19Z

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

        {
            var referenceManager = GetBoundReferenceManager();

            int length = referenceManager.ReferencedAssemblies.Length;

-            assemblies.EnsureCapacity(assemblies.Count + length);


why remove this? lists can have their capacity ensured as well, right?

Yes, but it doesn't really provide any value for a pooled object. Letting the array grow organically shouldn't show any allocation differences, assuming it goes back in the pool.

ToddGrun · 2025-08-18T14:01:56Z

@dotnet/roslyn-compiler -- ptal

AlekseyTs · 2025-08-19T03:33:41Z

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

@@ -30,6 +30,9 @@ internal abstract class AssemblySymbol : Symbol, IAssemblySymbolInternal
        // to the VB version.
        // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

+        // separate pool for assembly symbols as these collections commonly exceed ArrayBuilder's size threshold
+        private static readonly ObjectPool<List<AssemblySymbol>> s_symbolPool = new ObjectPool<List<AssemblySymbol>>(() => new List<AssemblySymbol>());


List

Can we still use a pool of ArrayBuilders, just with significantly big size threshold? #Closed

Can you clarify a bit? The reason I did it this way:

the discardLargeInstances version of ArrayBuilder.GetInstance isn't available within the compiler directory.

I'm not seeing the advantage of using ObjectPool<ArrayBuilder> rather than using an ObjectPool<List> (there are definitely more hits of the latter in the roslyn codebase than the former). If you know of an advantage, I'm definitely willing to investigate and change this.

Thank you for the clarification. The primary motivation for the question was the fact that keeping the same type would require less code changes at use sites.

the discardLargeInstances version of ArrayBuilder.GetInstance isn't available within the compiler directory.

Does ObjectPool<List<AssemblySymbol>> behave better in this regard?

I'm not seeing the advantage of using ObjectPool<ArrayBuilder> rather than using an ObjectPool<List>

What are the advantages of using ObjectPool<List> vs. ObjectPool<ArrayBuilder> in this case?

I'm sure it can work either way, but for me, ObjectPool and ArrayBuilder each have their own pooling semantics, and mixing them up confuses things (for me). As mentioned, ObjectPool<List> has more usage, including in the compilers folder, so it seemed like an appropriate choice.

My preference would be to not switch from an ArrayBuilder to List just because we want to use different pooling strategy.

Just to make sure we're on the same page:

The code before was using ArrayBuilder<AssemblySymbol> but that is a poor fit because of the size threshold ArrayBuilder uses and that the discardLargeInstances override isn't available in the compilers folder.

So, the code needed to be switched to another type, and specifically an ObjectPool of some collection type. To me, ObjectPool<ArrayBuilder> conflates multiple pooling technicques whereas ObjectPool<List> is commonly used in similar scenarios throughout the codebase.

If this were to use ObjectPool<ArrayBuilder>, the code would have to use new ArrayBuilder() in the pool to create it (not something code outside the ArrayBuilder class usually does) and it wouldn't call Free on it because the ArrayBuilder wouldn't be tied to the pool that that class knows about. I'm not seeing the benefit to wanting ObjectPool<ArrayBuilder> over ObjectPool<List>.

As I said, that concept is confusing to me, so I want to make sure I understand your proposal and it's value before trying something different from what is in this PR.

but that is a poor fit because of the size threshold ArrayBuilder uses and that the discardLargeInstances override isn't available in the compilers folder.

Is the issue with the collection behavior or the pooling strategy used by default? My current understanding is that the problem is with the default pooling strategy, not the collection behavior itself. Therefore, changing the collection type is not necessary and, in my opinion, is not justified.

If this were to use ObjectPool<ArrayBuilder>, the code would have to use new ArrayBuilder() in the pool to create it (not something code outside the ArrayBuilder class usually does) and it wouldn't call Free on it because the ArrayBuilder wouldn't be tied to the pool that that class knows about.

This doesn't scary me at all. If the problem is with the pooling strategy, that is what we should be fixing, and, if one place in code does that for ArrayBuilder, that is not a problem. We have plenty of places that use specialized (non-default) pools for various objects. Comments can help with explaining unusual logic.

I've gone ahead and pushed a change to use an ObjectPool<ArrayBuilder> instead of ObjectPool<List>. I personally find this more confusing than the original change (and think there is a reason basically no other code uses ArrayBuilders like this), but I don't think I'm able to convince you otherwise, and it's I don't think it woud be worthwhile to continue to try to do so. Let me know if this matches your expectation. Thanks!

and think there is a reason basically no other code uses ArrayBuilders like this

I think there is only one reason for that - the default pooling worked well. A lack of existing example on itself is not an indication that doing something is wrong, we are in the business of doing things that were never done before.

In general, decisions what collection to use and what pooling strategy to use are two distinct and independent decisions.

…er<AssemblySymbol>> per PR suggestion

… calling that as the ArrayBuilder implementation doesn't suffer from the same issues as the List implementation

AlekseyTs · 2025-09-08T16:44:13Z

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

@@ -1327,8 +1327,6 @@ internal void GetUnaliasedReferencedAssemblies(ArrayBuilder<AssemblySymbol> asse

            int length = referenceManager.ReferencedAssemblies.Length;

-            assemblies.EnsureCapacity(assemblies.Count + length);


assemblies.EnsureCapacity(assemblies.Count + length);

It is not obvious why this statement is removed. Do we have data proving that the removal has a noticeable positive impact? I would think that this statement was allowing us to reduce amount of times the builder was reallocated. Letting it grow on demand inside the loop might result in multiple reallocations. Assembly references with aliases are not very common, therefore, the used capacity should be pretty accurate. #Closed

Calling EnsureCapacity on a pooled data structure has virtually no benefit and could have a bad performance impact. For example, if the data structure continually grows by a constant number of elements, you end up reallocating each time, instead of taking advantage of the exponential growth that the Add call allows.

If the data structure isn't pooled, then I totally agree that ensuring capacity before calling Add makes sense.

Calling EnsureCapacity on a pooled data structure has virtually no benefit

Well, I described one possible benefit above.

and could have a bad performance impact. For example, if the data structure continually grows by a constant number of elements, you end up reallocating each time, instead of taking advantage of the exponential growth that the Add call allows.

This is a good theory. Do you have data supporting it for this specific code path, i.e. supporting a need for this change? If not, I am leaning towards reverting it.

I've seen such an allocation profile before, but I have no data on this particular call experiencing this. I've reverted the change. I cared enough to make what is almost certainly an improvement, but not enough to provide anything more than a simple explanation for why it's almost certainly a proper choice.

AlekseyTs · 2025-09-08T16:46:12Z

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

@@ -30,6 +30,9 @@ internal abstract class AssemblySymbol : Symbol, IAssemblySymbolInternal
        // to the VB version.
        // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

+        // separate pool for assembly symbols as these collections commonly exceed ArrayBuilder's size threshold


//

Consider using standard doc comment syntax #Closed

AlekseyTs · 2025-09-08T16:49:50Z

Done with review pass (commit 3)

AlekseyTs

LGTM (commit 5)

AlekseyTs · 2025-09-09T18:04:02Z

@ToddGrun Please squash commits while merging.

github-actions bot added the Area-Compilers label Aug 6, 2025

CyrusNajmabadi reviewed Aug 6, 2025

View reviewed changes

ToddGrun changed the title *** WIP: Reduce allocations in Assembly.GetTopLevelTypeByMetadataName Reduce allocations in AssemblySymbol.GetTopLevelTypeByMetadataName Aug 7, 2025

ToddGrun marked this pull request as ready for review August 7, 2025 00:25

ToddGrun requested a review from a team as a code owner August 7, 2025 00:25

333fred approved these changes Aug 19, 2025

View reviewed changes

AlekseyTs reviewed Aug 19, 2025

View reviewed changes

CyrusNajmabadi approved these changes Aug 26, 2025

View reviewed changes

ToddGrun added 2 commits September 8, 2025 08:25

Change from ObjectPool<List<AssemblySymbol>> to ObjectPool<ArrayBuild…

598f534

…er<AssemblySymbol>> per PR suggestion

Change back to call AddRange as it's we would no longer need to avoid…

dd7d212

… calling that as the ArrayBuilder implementation doesn't suffer from the same issues as the List implementation

AlekseyTs reviewed Sep 8, 2025

View reviewed changes

ToddGrun added 2 commits September 8, 2025 10:44

Use doc comment

f22bf15

Revert a good change due to pr comments

b6f509c

AlekseyTs approved these changes Sep 9, 2025

View reviewed changes

ToddGrun merged commit 3faf4ab into dotnet:main Sep 9, 2025
24 checks passed

dotnet-policy-service bot added this to the Next milestone Sep 9, 2025

		@@ -1327,8 +1327,6 @@ internal void GetUnaliasedReferencedAssemblies(ArrayBuilder<AssemblySymbol> asse

		int length = referenceManager.ReferencedAssemblies.Length;

		assemblies.EnsureCapacity(assemblies.Count + length);

Reduce allocations in AssemblySymbol.GetTopLevelTypeByMetadataName #79810

Reduce allocations in AssemblySymbol.GetTopLevelTypeByMetadataName #79810

Conversation

ToddGrun commented Aug 6, 2025 • 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!

ToddGrun commented Aug 18, 2025

Uh oh!

AlekseyTs Aug 19, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

ToddGrun Aug 28, 2025 • 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!

AlekseyTs Aug 29, 2025 • 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!

ToddGrun Aug 29, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Sep 8, 2025 • 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!

AlekseyTs Sep 8, 2025 • 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!

AlekseyTs Sep 8, 2025 • 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!

AlekseyTs commented Sep 8, 2025

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Sep 9, 2025

Uh oh!

Uh oh!

Uh oh!

ToddGrun commented Aug 6, 2025 •

edited

Loading

AlekseyTs Aug 19, 2025 •

edited

Loading

ToddGrun Aug 28, 2025 •

edited

Loading

AlekseyTs Aug 29, 2025 •

edited

Loading

ToddGrun Aug 29, 2025 •

edited

Loading

AlekseyTs Sep 8, 2025 •

edited

Loading

AlekseyTs Sep 8, 2025 •

edited

Loading

AlekseyTs Sep 8, 2025 •

edited

Loading