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.

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

Conversation

ToddGrun
Copy link
Contributor

  1. No need to create a builder if _projectIdToTrackerMap is empty
  2. No need to create a new ImmutableDictionary if it would end up having the exact same contents as projectIdToTrackerMap

1) No need to create a builder if _projectIdToTrackerMap is empty
2) No need to create a new ImmutableDictionary if it would end up having the exact same contents as projectIdToTrackerMap
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 10, 2023
@ToddGrun ToddGrun marked this pull request as ready for review October 10, 2023 17:50
@ToddGrun ToddGrun requested a review from a team as a code owner October 10, 2023 17:50

var newTrackerInfo = ArrayBuilder<KeyValuePair<ProjectId, ICompilationTracker>>.GetInstance();
var allReused = true;
Copy link
Member

@jasonmalinowski jasonmalinowski Oct 10, 2023

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.

@ToddGrun
Copy link
Contributor Author

1 similar comment
@ToddGrun
Copy link
Contributor Author

@ToddGrun ToddGrun merged commit 63cbcad into dotnet:main Oct 12, 2023
24 checks passed
@ghost ghost added this to the Next milestone Oct 12, 2023
@jjonescz jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants