Reduce allocations again in ComputeReverseReferencesMap #76417
Conversation
Speedometer results from the last PR didn't improve allocations in the ProjectDependencyGraph as much as I had hoped. This PR instead changes ComputeReverseReferencesMap to use pooled data structures and ImmutableHashSet.Builder to build up it's final result. The speedometer test is still showing about 100 MB of allocations in this method during typing, my hope is that this should cut that in about half. Will create a test insertion for validation.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
Commit 1 Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/598416 |
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/ProjectDependencyGraph.cs
Outdated
Show resolved
Hide resolved
2) Increase pool size from 128 to 256 as that's not an uncommon number of projects
| return KeyValuePairUtil.Create(kvp.Key, reverseReferencesForProject); | ||
| }) | ||
| .ToImmutableDictionary(); | ||
| return reverseReferencesMap.ToImmutableDictionary(); |
There was a problem hiding this comment.
poooled dict, to immutable dict? shouldn't this just be an ImmutableDictBuilder?
There was a problem hiding this comment.
It could be, but the builder is a class itself, so this avoids that allocation. I can switch to that though if you prefer.
There was a problem hiding this comment.
teh main concern here is that to go from Dict to ImmutableDict you are rehashing everything.
Note: you could pool the ImmutableDictBuilder if you want to save that allocation.
There was a problem hiding this comment.
I'll just switch to an ImmutableDictBuilder here and see what the test insertion says.
There was a problem hiding this comment.
Insertions says the allocation of the dictionary builder vs using a pooled dictionary doesn't show up, so we'll just go with this approach
|
Commit 3 test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/598548 |
Speedometer results from the last PR didn't improve allocations in the ProjectDependencyGraph as much as I had hoped. This PR instead changes ComputeReverseReferencesMap to use pooled data structures and ImmutableHashSet.Builder to build up it's final result.
The speedometer test is still showing about 100 MB of allocations in this method during typing, my hope is that this should cut that in about half. Will create a test insertion for validation.
*** allocations that are being targeted by this PR ***
