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

Jump to bottom

Avoid unnecessary project enumeration in SolutionState.Branch #53810

Closed
wants to merge 4 commits into from
Closed

Avoid unnecessary project enumeration in SolutionState.Branch #53810

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
47c3c7b
Avoid unnecessary project enumeration in SolutionState.Branch
mavasani Jun 1, 2021
e7c423d
Remove parens
mavasani Jun 1, 2021
af5c0f4
Address feedback - use projectIds
mavasani Jun 2, 2021
2e1e0f0
Fix a bug in SerializableOptionSet.WithChangeOption found by the new …
mavasani Jun 3, 2021
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
8 changes: 7 additions & 1 deletion 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 @@ -220,13 +220,19 @@ private SolutionState Branch(
solutionAttributes ??= _solutionAttributes;
projectIds ??= ProjectIds;
idToProjectStateMap ??= _projectIdToProjectStateMap;
options ??= Options.WithLanguages(GetRemoteSupportedProjectLanguages(idToProjectStateMap));
mavasani marked this conversation as resolved.
Show resolved Hide resolved
analyzerReferences ??= AnalyzerReferences;
projectIdToTrackerMap ??= _projectIdToTrackerMap;
filePathToDocumentIdsMap ??= _filePathToDocumentIdsMap;
dependencyGraph ??= _dependencyGraph;
var newFrozenSourceGeneratedDocumentState = frozenSourceGeneratedDocument.HasValue ? frozenSourceGeneratedDocument.Value : _frozenSourceGeneratedDocumentState;

// PERF: Only invoke WithLanguages if we have different set of project IDs (AddProject/RemoveProject operation)
Copy link
Member

Choose a reason for hiding this comment

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

seems sane. but i worry if it's possible to somehow have an Add+REmove processed at teh same time that woudl lead us to have the same count. @jasonmalinowski for validation that we do things in tiny steps, so this sort of optimization is ok.

Copy link
Contributor Author

@mavasani mavasani Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I audited the existing callsites to validate that they satisfy the first assertion. Definitely need @jasonmalinowski's confirmation on this one though.

Copy link
Member

Choose a reason for hiding this comment

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

So for some reason we also have a ProjectIds collection we pass around, and that's immutable. I'm guessing you're safe to do the WithLanguages any time the ProjectId collection object reference changes. The check is won't have false negatives where we fail to rerun WithLanguages; it could theoretically have a false positive where might run it if we manipulate the list and end up with the same original semantic value but I don't think we do that anywhere. (Or if we do, it's fine enough to not care since we'll still get quite the win.)

Copy link
Contributor Author

@mavasani mavasani Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, that seems to be a much better approach. Let me adjust.

Debug.Assert(projectIds != ProjectIds ||
Options == Options.WithLanguages(GetRemoteSupportedProjectLanguages(idToProjectStateMap)));
options ??= projectIds != ProjectIds
? Options.WithLanguages(GetRemoteSupportedProjectLanguages(idToProjectStateMap))
: Options;

var analyzerReferencesEqual = AnalyzerReferences.SequenceEqual(analyzerReferences);

if (branchId == _branchId &&
Expand Down