Skip to content
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.

Sign up for GitHub

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

Reduce ImmutableDictionary allocations in CreateCompilationTrackerMap #70319

Merged
merged 3 commits into from
Oct 12, 2023
Merged

Reduce ImmutableDictionary allocations in CreateCompilationTrackerMap #70319

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77b4ead
Reduce ImmutableDictionary allocations in CreateCompilationTrackerMap
ToddGrun Oct 10, 2023
2ac7cd4
Use ArrayBuilder instead of allocating a new list each time
ToddGrun Oct 10, 2023
fe911b1
Forgot to add the using around the ArrayBuilder call
ToddGrun Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1549,12 +1549,27 @@ private static ProjectDependencyGraph CreateDependencyGraph(

private ImmutableDictionary<ProjectId, ICompilationTracker> CreateCompilationTrackerMap(ProjectId changedProjectId, ProjectDependencyGraph dependencyGraph)
{
var builder = ImmutableDictionary.CreateBuilder<ProjectId, ICompilationTracker>();
if (_projectIdToTrackerMap.Count == 0)
return _projectIdToTrackerMap;

var newTrackerInfo = ArrayBuilder<KeyValuePair<ProjectId, ICompilationTracker>>.GetInstance();
sharwell marked this conversation as resolved.
Show resolved Hide resolved
var allReused = true;
Copy link
Member

@jasonmalinowski jasonmalinowski Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we better off just doing a loop first checking the dependency graph CanReuse() for each of the entries, and if so just bailing entirely? Otherwise here we're still allocating the builder only to throw it away if that case happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I've seen, pretty much everything about ImmutableDictionarys are slow, so I figured it was just better to make a single pass through it. The intent here is that the ArrayBuilder won't actually allocate normally, so the only real allocation that will happen is if the ImmutableDictionary really needs to be created.

foreach (var (id, tracker) in _projectIdToTrackerMap)
builder.Add(id, CanReuse(id) ? tracker : tracker.Fork(tracker.ProjectState, translate: null));
{
var localTracker = tracker;
if (!CanReuse(id))
{
localTracker = tracker.Fork(tracker.ProjectState, translate: null);
allReused = false;
}

return builder.ToImmutable();
newTrackerInfo.Add(new KeyValuePair<ProjectId, ICompilationTracker>(id, localTracker));
}

if (allReused)
return _projectIdToTrackerMap;

return ImmutableDictionary.CreateRange(newTrackerInfo);

// Returns true if 'tracker' can be reused for project 'id'
bool CanReuse(ProjectId id)
Expand Down