From a0abb8fe33aa85b4c157d7dc16b52409fc0e9af8 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 14:14:21 -0800 Subject: [PATCH 1/8] Fix issue where we were trying to freeze non-C#/VB projects --- .../Portable/Workspace/Solution/SolutionCompilationState.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs index f0ab0dfbb98cc..f032817501b89 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs @@ -1071,7 +1071,10 @@ private SolutionCompilationState ComputeFrozenSnapshot(CancellationToken cancell { cancellationToken.ThrowIfCancellationRequested(); - // if we don't have one or it is stale, create a new partial solution + var oldProjectState = this.SolutionState.GetRequiredProjectState(projectId); + if (!oldProjectState.SupportsCompilation) + continue; + var oldTracker = GetCompilationTracker(projectId); var newTracker = oldTracker.FreezePartialState(cancellationToken); if (oldTracker == newTracker) @@ -1079,7 +1082,6 @@ private SolutionCompilationState ComputeFrozenSnapshot(CancellationToken cancell Contract.ThrowIfFalse(newIdToProjectStateMapBuilder.ContainsKey(projectId)); - var oldProjectState = this.SolutionState.GetRequiredProjectState(projectId); var newProjectState = newTracker.ProjectState; newIdToProjectStateMapBuilder[projectId] = newProjectState; From 357696c055f9ddc6b01318253d75facb76aac426 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 14:20:28 -0800 Subject: [PATCH 2/8] Add test --- .../CoreTest/SolutionTests/SolutionTests.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs index f99467d9ab0b0..d057dfb9bc7db 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs @@ -4719,5 +4719,22 @@ public async Task TestFrozenPartialSolution7(bool freeze) Assert.Equal("// source2", forkedGeneratedDocuments.Single().GetTextSynchronously(CancellationToken.None).ToString()); } } + + [Fact] + public async Task TestFrozenPartialSolutionOtherLanguage() + { + using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartialSemantics(); + var project = workspace.CurrentSolution.AddProject("TypeScript", "TypeScript", "TypeScript"); + project = project.AddDocument("Extra.ts", SourceText.From("class Extra { }")).Project; + + // Because we froze before ever even looking at anything semantics related, we should have no documents in + // this project. + var frozenSolution = project.Solution.WithFrozenPartialCompilations(CancellationToken.None); + var frozenProject = frozenSolution.Projects.Single(); + Assert.Single(frozenProject.Documents); + + var frozenCompilation = await frozenProject.GetCompilationAsync(); + Assert.Null(frozenCompilation); + } } } From 0c70386da57012cad5e5b0f93ef86fea81fe0ea0 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 14:21:20 -0800 Subject: [PATCH 3/8] Comment --- src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs index d057dfb9bc7db..bb890884334b4 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs @@ -4727,8 +4727,7 @@ public async Task TestFrozenPartialSolutionOtherLanguage() var project = workspace.CurrentSolution.AddProject("TypeScript", "TypeScript", "TypeScript"); project = project.AddDocument("Extra.ts", SourceText.From("class Extra { }")).Project; - // Because we froze before ever even looking at anything semantics related, we should have no documents in - // this project. + // Freeze should have no impact on non-c#/vb projects. var frozenSolution = project.Solution.WithFrozenPartialCompilations(CancellationToken.None); var frozenProject = frozenSolution.Projects.Single(); Assert.Single(frozenProject.Documents); From bcf374121b1e04d6db08a1738c8cf5a872ff1c27 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 14:34:27 -0800 Subject: [PATCH 4/8] Avoid calls --- .../Core/Portable/Workspace/Solution/SolutionState.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs index 8a6fb868502e4..69cbd3af50fcc 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs @@ -417,8 +417,11 @@ public ImmutableDictionary> CreateFilePathToD // the entry entirely for it in the dictionary, only to add it back in. Adding then removing will at least // keep the entry, but increase the docs for it, then lower it back down. - AddDocumentFilePaths(documentsToAdd, builder); - RemoveDocumentFilePaths(documentsToRemove, builder); + if (documentsToAdd.Count > 0) + AddDocumentFilePaths(documentsToAdd, builder); + + if (documentsToRemove.Count > 0) + RemoveDocumentFilePaths(documentsToRemove, builder); return builder.ToImmutable(); } From d2044975a448e153c50acd018e9a8eea83e296b4 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 15:04:27 -0800 Subject: [PATCH 5/8] Avoid intermediary arrays --- .../Solution/SolutionCompilationState.cs | 26 +++++--- .../Workspace/Solution/SolutionState.cs | 63 +++++++------------ 2 files changed, 38 insertions(+), 51 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs index f032817501b89..1718347255342 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs @@ -1064,8 +1064,8 @@ private SolutionCompilationState ComputeFrozenSnapshot(CancellationToken cancell var newIdToProjectStateMapBuilder = this.SolutionState.ProjectStates.ToBuilder(); var newIdToTrackerMapBuilder = _projectIdToTrackerMap.ToBuilder(); - using var _1 = ArrayBuilder.GetInstance(out var documentsToRemove); - using var _2 = ArrayBuilder.GetInstance(out var documentsToAdd); + var filePathToDocumentIdsMapBuilder = this.SolutionState.FilePathToDocumentIdsMap.ToBuilder(); + var filePathToDocumentIdsMapChanged = false; foreach (var projectId in this.SolutionState.ProjectIds) { @@ -1112,9 +1112,10 @@ private SolutionCompilationState ComputeFrozenSnapshot(CancellationToken cancell var newIdToProjectStateMap = newIdToProjectStateMapBuilder.ToImmutable(); var newIdToTrackerMap = newIdToTrackerMapBuilder.ToImmutable(); - var filePathToDocumentIdsMap = this.SolutionState.CreateFilePathToDocumentIdsMapWithAddedAndRemovedDocuments( - documentsToAdd: documentsToAdd, - documentsToRemove: documentsToRemove); + var filePathToDocumentIdsMap = filePathToDocumentIdsMapChanged + ? filePathToDocumentIdsMapBuilder.ToImmutable() + : null; + var dependencyGraph = SolutionState.CreateDependencyGraph(this.SolutionState.ProjectIds, newIdToProjectStateMap); var newState = this.SolutionState.Branch( @@ -1135,11 +1136,18 @@ void CheckDocumentStates( TextDocumentStates newStates) where TDocumentState : TextDocumentState { // Get the trivial sets of documents that are present in one set but not the other. + foreach (var documentId in newStates.GetAddedStateIds(oldStates)) - documentsToAdd.Add(newStates.GetRequiredState(documentId)); + { + filePathToDocumentIdsMapChanged = true; + SolutionState.AddDocumentFilePath(newStates.GetRequiredState(documentId), filePathToDocumentIdsMapBuilder); + } foreach (var documentId in newStates.GetRemovedStateIds(oldStates)) - documentsToRemove.Add(oldStates.GetRequiredState(documentId)); + { + filePathToDocumentIdsMapChanged = true; + SolutionState.RemoveDocumentFilePath(oldStates.GetRequiredState(documentId), filePathToDocumentIdsMapBuilder); + } // Now go through the states that are in both sets. We have to check these all as it is possible for // document to change its file path without its id changing. @@ -1149,8 +1157,8 @@ void CheckDocumentStates( oldDocumentState != newDocumentState && oldDocumentState.FilePath != newDocumentState.FilePath) { - documentsToRemove.Remove(oldDocumentState); - documentsToAdd.Add(newDocumentState); + SolutionState.RemoveDocumentFilePath(oldDocumentState, filePathToDocumentIdsMapBuilder); + SolutionState.AddDocumentFilePath(newDocumentState, filePathToDocumentIdsMapBuilder); } } } diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs index 69cbd3af50fcc..5e26a3538c837 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs @@ -220,6 +220,8 @@ public SolutionState WithNewWorkspace( _lazyAnalyzers); } + public ImmutableDictionary> FilePathToDocumentIdsMap => _filePathToDocumentIdsMap; + /// /// The version of the most recently modified project. /// @@ -404,28 +406,6 @@ public SolutionState RemoveProject(ProjectId projectId) dependencyGraph: newDependencyGraph); } - public ImmutableDictionary> CreateFilePathToDocumentIdsMapWithAddedAndRemovedDocuments( - ArrayBuilder documentsToAdd, - ArrayBuilder documentsToRemove) - { - if (documentsToRemove.Count == 0 && documentsToAdd.Count == 0) - return _filePathToDocumentIdsMap; - - var builder = _filePathToDocumentIdsMap.ToBuilder(); - - // Add first, then remove. This helps avoid the case where a filepath now sees no documents, so we remove - // the entry entirely for it in the dictionary, only to add it back in. Adding then removing will at least - // keep the entry, but increase the docs for it, then lower it back down. - - if (documentsToAdd.Count > 0) - AddDocumentFilePaths(documentsToAdd, builder); - - if (documentsToRemove.Count > 0) - RemoveDocumentFilePaths(documentsToRemove, builder); - - return builder.ToImmutable(); - } - public ImmutableDictionary> CreateFilePathToDocumentIdsMapWithAddedDocuments(IEnumerable documentStates) { var builder = _filePathToDocumentIdsMap.ToBuilder(); @@ -436,16 +416,17 @@ public ImmutableDictionary> CreateFilePathToD private static void AddDocumentFilePaths(IEnumerable documentStates, ImmutableDictionary>.Builder builder) { foreach (var documentState in documentStates) - { - var filePath = documentState.FilePath; + AddDocumentFilePath(documentState, builder); + } - if (RoslynString.IsNullOrEmpty(filePath)) - { - continue; - } + public static void AddDocumentFilePath(TextDocumentState documentState, ImmutableDictionary>.Builder builder) + { + var filePath = documentState.FilePath; - builder.MultiAdd(filePath, documentState.Id); - } + if (RoslynString.IsNullOrEmpty(filePath)) + return; + + builder.MultiAdd(filePath, documentState.Id); } public ImmutableDictionary> CreateFilePathToDocumentIdsMapWithRemovedDocuments(IEnumerable documentStates) @@ -458,21 +439,19 @@ public ImmutableDictionary> CreateFilePathToD private static void RemoveDocumentFilePaths(IEnumerable documentStates, ImmutableDictionary>.Builder builder) { foreach (var documentState in documentStates) - { - var filePath = documentState.FilePath; + RemoveDocumentFilePath(documentState, builder); + } - if (RoslynString.IsNullOrEmpty(filePath)) - { - continue; - } + public static void RemoveDocumentFilePath(TextDocumentState documentState, ImmutableDictionary>.Builder builder) + { + var filePath = documentState.FilePath; + if (RoslynString.IsNullOrEmpty(filePath)) + return; - if (!builder.TryGetValue(filePath, out var documentIdsWithPath) || !documentIdsWithPath.Contains(documentState.Id)) - { - throw new ArgumentException($"The given documentId was not found in '{nameof(_filePathToDocumentIdsMap)}'."); - } + if (!builder.TryGetValue(filePath, out var documentIdsWithPath) || !documentIdsWithPath.Contains(documentState.Id)) + throw new ArgumentException($"The given documentId was not found in '{nameof(_filePathToDocumentIdsMap)}'."); - builder.MultiRemove(filePath, documentState.Id); - } + builder.MultiRemove(filePath, documentState.Id); } private ImmutableDictionary> CreateFilePathToDocumentIdsMapWithFilePath(DocumentId documentId, string? oldFilePath, string? newFilePath) From a6e42490eb6097e5832a964c262e7106892fafd1 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 15:07:10 -0800 Subject: [PATCH 6/8] fast path out --- .../Workspace/Solution/SolutionCompilationState.cs | 3 +++ .../Portable/Workspace/Solution/TextDocumentStates.cs | 10 +++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs index 1718347255342..2591b42b06183 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs @@ -1135,6 +1135,9 @@ void CheckDocumentStates( TextDocumentStates oldStates, TextDocumentStates newStates) where TDocumentState : TextDocumentState { + if (oldStates.Equals(newStates)) + return; + // Get the trivial sets of documents that are present in one set but not the other. foreach (var documentId in newStates.GetAddedStateIds(oldStates)) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs b/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs index 2df39ead6024b..eade515ce879d 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs @@ -11,7 +11,6 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Serialization; using Roslyn.Utilities; @@ -217,6 +216,15 @@ private static IEnumerable Except(ImmutableList ids, Imm public bool HasAnyStateChanges(TextDocumentStates oldStates) => !_map.Values.SequenceEqual(oldStates._map.Values); + public override bool Equals(object obj) + => obj is TextDocumentStates other && Equals(other); + + public override int GetHashCode() + => throw new NotSupportedException(); + + public bool Equals(TextDocumentStates other) + => _map == other._map && _ids == other.Ids; + private sealed class DocumentIdComparer : IComparer { public static readonly IComparer Instance = new DocumentIdComparer(); From 1d6da389b42027b3edcf6af5e427e3c79545371b Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 15:13:52 -0800 Subject: [PATCH 7/8] nrt --- .../Core/Portable/Workspace/Solution/TextDocumentStates.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs b/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs index eade515ce879d..723edf2ac17fc 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs @@ -216,7 +216,7 @@ private static IEnumerable Except(ImmutableList ids, Imm public bool HasAnyStateChanges(TextDocumentStates oldStates) => !_map.Values.SequenceEqual(oldStates._map.Values); - public override bool Equals(object obj) + public override bool Equals(object? obj) => obj is TextDocumentStates other && Equals(other); public override int GetHashCode() From ae884df1d9e15fccfcb366e9ab13c67efb57e2c9 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Mon, 26 Feb 2024 15:37:00 -0800 Subject: [PATCH 8/8] Docs --- .../Portable/Workspace/Solution/SolutionCompilationState.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs index 2591b42b06183..1a71118b9cebc 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs @@ -1071,6 +1071,7 @@ private SolutionCompilationState ComputeFrozenSnapshot(CancellationToken cancell { cancellationToken.ThrowIfCancellationRequested(); + // Definitely do nothing for non-C#/VB projects. We have nothing to freeze in that case. var oldProjectState = this.SolutionState.GetRequiredProjectState(projectId); if (!oldProjectState.SupportsCompilation) continue; @@ -1160,6 +1161,7 @@ void CheckDocumentStates( oldDocumentState != newDocumentState && oldDocumentState.FilePath != newDocumentState.FilePath) { + filePathToDocumentIdsMapChanged = true; SolutionState.RemoveDocumentFilePath(oldDocumentState, filePathToDocumentIdsMapBuilder); SolutionState.AddDocumentFilePath(newDocumentState, filePathToDocumentIdsMapBuilder); }