Reduce allocations in SolutionCompilationState.CreateCompilationTrackerMap #72596
Conversation
|
PIT run numbers that came through the test insertion look really good. Promoting this PR. |
| @@ -31,6 +32,8 @@ internal sealed partial class SolutionCompilationState | |||
| /// </summary> | |||
| private static readonly ConditionalWeakTable<ISymbol, ProjectId> s_assemblyOrModuleSymbolToProjectMap = new(); | |||
|
|
|||
| private static readonly ObjectPool<SegmentedDictionary<ProjectId, ICompilationTracker>> s_trackerInfoPool = new(() => new()); | |||
There was a problem hiding this comment.
❓ Can you run another pass through without this pool? Instead of using an intermediate dictionary, just use _projectIdToTrackerMap.ToBuilder() at the start of the method, and then return builder.ToImmutable() at the end. This will always create a small builder object, but it removes the need for allReused and isModified, and only allocates a new dictionary if/when something changes.
There was a problem hiding this comment.
Sure, I can give that a go, results probably won't come out for around 12 hours.
From my debugging, I saw a large number of calls during project open come through where projectIdToTrackerMap was empty, so I don't love that this would allocate, but we'll give it a go.
There was a problem hiding this comment.
From my debugging, I saw a large number of calls during project open come through where projectIdToTrackerMap was empty
Thanks, this is very helpful. I reviewed all 5 call sites based on this, and found:
- 3 call sites never add new items to the map, and would silently proceed if the map was empty. These call sites can pass
skipEmpty: true(name could be changed), allowing the call to return immediately if the map is empty. - 1 call site (
WithFrozenSourceGeneratedDocuments) will add items to the map in the callback. This call site can passskipEmpty: false. - 1 call site (
WithoutFrozenSourceGeneratedDocuments) does not add items to the map, but it has a runtime assertion that the map is not empty in the callback. This call site can also passskipEmpty: falseto preserve the assertion behavior.
When skipEmpty is true, return _projectIdToTrackerMap without any other work if it is empty at the start of the method. Otherwise, proceed with the ToBuilder() and the builder will perform the modification detection.
|
@sharwell -- Went ahead with the SegmentedCollectionsMarshal usage directly at this location. |
…tionTracker>.Builder 2) Change the modification callback to an action 3) Pass flag indicating whether callback needs to happen on empty collections
src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
Outdated
Show resolved
Hide resolved
…ationState.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
…utableSegmentedDictionary
|
@sharwell -- assuming the speedometer tests come back positive, any remaining concerns? |
|
No further concerns |
|
speedometer run was broken, don't really want to wait another 12 hours for another chance. I'm pretty certain this should have a sizable positive impact, so I'm going to go ahead and insert once tests pass on the ci. |
…utableSegmentedDictionary
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
…utableSegmentedDictionary
Reduce allocations due to resizes in SolutionCompilationState.CreateCompilationTrackerMap. We now use an ImmutableSegmentedDictionary builder to both simplify the code and allocate less. The builder will only create a new dictionary if it's modified, and it does so in a way that doesn't require multiple resizes (which is where the allocation gains from this change are realized).
Additionally, use of the builder allowed a simplification where the modification callback can be changed to a simple action. The call to CreateCompilationTrackerMap now passes in an optimization flag indicating whether the callback needs to occur for empty collections.