Remove BranchId from the workspace. #63578
Conversation
| @@ -130,9 +130,6 @@ private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs e) | |||
| private async ValueTask SynchronizePrimaryWorkspaceAsync(CancellationToken cancellationToken) | |||
| { | |||
| var solution = _workspace.CurrentSolution; | |||
| if (solution.BranchId != _workspace.PrimaryBranchId) | |||
| return; | |||
There was a problem hiding this comment.
totally unnecessary. we are always accessing _workspace.CurrentSolution above. So this solution was always the primary-branch solution.
| private readonly ConditionalWeakTable<Solution, Solution> _designTimeToCompileTimeSolution = new(); | ||
| #else | ||
| private ConditionalWeakTable<Solution, Solution> _designTimeToCompileTimeSolution = new(); | ||
| #endif |
There was a problem hiding this comment.
.net core has ConditionalWeakTable.Clear(). Standard does not. So we simulate it by overwriting the field.
| /// Cached compile time solution for a forked branch. This is used primarily by LSP cases where | ||
| /// we fork the workspace solution and request diagnostics for the forked solution. | ||
| /// </summary> | ||
| private (int DesignTimeSolutionVersion, BranchId DesignTimeSolutionBranch, Solution CompileTimeSolution)? _forkedBranchCompileTimeCache; |
There was a problem hiding this comment.
@dibarbet i believe this has the semantics from before. Previously we keyed basd on Version+BranchId. But these change every time you get a new solution. So the only time these woudl match would be if you passed in teh exact same (reference-equals) Solution instance from before. So we can get the same behavior by using a CWT keyed off Solution instances.
There was a problem hiding this comment.
The idea here being that as soon as no one is using the solution we can get rid of the design time solution. Makes sense to me.
| @@ -57,8 +57,7 @@ internal class DiagnosticComputer | |||
| _analysisKind = analysisKind; | |||
| _analyzerInfoCache = analyzerInfoCache; | |||
|
|
|||
| // We only track performance from primary branch. All forked branch we don't care such as preview. | |||
| _performanceTracker = project.IsFromPrimaryBranch() ? project.Solution.Services.GetService<IPerformanceTrackerService>() : null; | |||
| _performanceTracker = project.Solution.Services.GetRequiredService<IPerformanceTrackerService>(); | |||
There was a problem hiding this comment.
i don't see the value that in this. Why would it not matter what the performance was getting diagnostics in a preview workspace?
| { | ||
| CheckNoChanges(oldSolution, newSolution); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
also not certain what teh value here is. If someone asks us to apply changes, we can simply walk through and figure out waht the changes were. This was a fast-bail-out for a case that seems like the unlikely one (e.g. when does someone ask to apply the same solution over the exisitng solution)?
we could choose to still haev a check like if (oldSolution == newSolution). but i question if htat has any value whatsoever given that newSolution.GetChanges(oldSolution) will just return an empty set of changes.
| #if NETCOREAPP | ||
| _designTimeToCompileTimeSolution.Clear(); | ||
| #else | ||
| _designTimeToCompileTimeSolution = new(); |
There was a problem hiding this comment.
note: doing the above seems unnecessary. as a CWT, they should naturally release their resources when they are no longer referenced anywhere.
|
@dibarbet @jasonmalinowski ptal. |
| /// Cached compile time solution for a forked branch. This is used primarily by LSP cases where | ||
| /// we fork the workspace solution and request diagnostics for the forked solution. | ||
| /// </summary> | ||
| private (int DesignTimeSolutionVersion, BranchId DesignTimeSolutionBranch, Solution CompileTimeSolution)? _forkedBranchCompileTimeCache; |
There was a problem hiding this comment.
The idea here being that as soon as no one is using the solution we can get rid of the design time solution. Makes sense to me.
ghost
added this to the 
Can choose to review this commit by commit, or just the end form.
The BranchId concept has always been somewhat suspect. It was a way of determining if the Solution instance you had was a Solution that had ever been teh .CurrentSolution of a workspace, or if you had a forked solution off of one of those solutions. Needing to know if you were one of these has never been clear as to why that's important. Roslyn operates in snapshots, and attempts to do so in a purely functional fashion. What does it matter if you were on a .CurrentSolution, or a fork of that, or a fork of that fork?
At one point we definitely used this when it came to persisting data. We didn't want analysis about a fork of a solution to overwrite data in our persistence service corresponding to the primary solution. However, that code went away ages ago when:
At that point, there was no concern about overwriting data with something invalid.
This PR gets rid of hte BranchId concept entirely and fixes up any remaining stragglers to get off of it.