Skip to content

Commit 991f327

Browse files
authored
Fine tuning of what types of project update affect what state (#11213)
Follow up to #11191 which caused RPS regressions. There were some edge-y things I papered over in that PR, assuming they were unnecessary over-complications, but given the RPS regression I had a closer look, and they could have caused re-compilation of closed documents when only tag helpers changed, which would be new behaviour. Not _entirely_ convinced the re-compilation is all unnecessary (if a document used a Tag Helper that has only just been discovered, then its code gen would legitimately change) but it's hard to justify when I can't point to any tooling for the closed files that would actually benefit from the extra work. The key thing is not changing `DocumentCollectionVersion` when only `ProjectWorkspaceState` changes. Not re-calculating import documents was just a little extra amuse-bouche, and not caching the computed state tracker is pure paranoia.
2 parents ade9db7 + 39e8d1f commit 991f327

File tree

4 files changed

+71
-18
lines changed

4 files changed

+71
-18
lines changed

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -160,12 +160,14 @@ public virtual DocumentState WithImportsChange()
160160
return state;
161161
}
162162

163-
public virtual DocumentState WithProjectChange()
163+
public virtual DocumentState WithProjectChange(bool cacheComputedState)
164164
{
165165
var state = new DocumentState(HostDocument, Version + 1, _textAndVersion, _textLoader);
166166

167-
// Optimistically cache the computed state
168-
state._computedState = new ComputedStateTracker(_computedState);
167+
if (cacheComputedState)
168+
{
169+
state._computedState = new ComputedStateTracker(_computedState);
170+
}
169171

170172
return state;
171173
}

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -274,26 +274,36 @@ public ProjectState WithChangedHostDocument(HostDocument hostDocument, TextLoade
274274

275275
public ProjectState WithHostProjectAndWorkspaceState(HostProject hostProject, ProjectWorkspaceState projectWorkspaceState)
276276
{
277-
if (HostProject.Configuration.Equals(hostProject.Configuration) &&
278-
HostProject.RootNamespace == hostProject.RootNamespace &&
279-
ProjectWorkspaceState.Equals(projectWorkspaceState))
277+
var configUnchanged = (HostProject.Configuration.Equals(hostProject.Configuration) &&
278+
HostProject.RootNamespace == hostProject.RootNamespace);
279+
280+
if (configUnchanged && ProjectWorkspaceState.Equals(projectWorkspaceState))
280281
{
281282
return this;
282283
}
283284

284-
var documents = Documents.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.WithProjectChange(), FilePathNormalizingComparer.Instance);
285+
var documents = Documents.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.WithProjectChange(cacheComputedState: configUnchanged), FilePathNormalizingComparer.Instance);
285286

286287
// If the host project has changed then we need to recompute the imports map
287-
var importsToRelatedDocuments = s_emptyImportsToRelatedDocuments;
288+
var importsToRelatedDocuments = configUnchanged
289+
? ImportsToRelatedDocuments
290+
: ComputeImportsToRelatedDocuments(documents);
291+
292+
var state = new ProjectState(this, numberOfDocumentsMayHaveChanged: !configUnchanged, hostProject, projectWorkspaceState, documents, importsToRelatedDocuments);
293+
return state;
288294

289-
foreach (var document in documents)
295+
ImmutableDictionary<string, ImmutableArray<string>> ComputeImportsToRelatedDocuments(ImmutableDictionary<string, DocumentState> documents)
290296
{
291-
var importTargetPaths = GetImportDocumentTargetPaths(document.Value.HostDocument);
292-
importsToRelatedDocuments = AddToImportsToRelatedDocuments(importsToRelatedDocuments, document.Value.HostDocument.FilePath, importTargetPaths);
293-
}
297+
var importsToRelatedDocuments = s_emptyImportsToRelatedDocuments;
294298

295-
var state = new ProjectState(this, numberOfDocumentsMayHaveChanged: true, hostProject, projectWorkspaceState, documents, importsToRelatedDocuments);
296-
return state;
299+
foreach (var document in documents)
300+
{
301+
var importTargetPaths = GetImportDocumentTargetPaths(document.Value.HostDocument);
302+
importsToRelatedDocuments = AddToImportsToRelatedDocuments(importsToRelatedDocuments, document.Value.HostDocument.FilePath, importTargetPaths);
303+
}
304+
305+
return importsToRelatedDocuments;
306+
}
297307
}
298308

299309
internal static ImmutableDictionary<string, ImmutableArray<string>> AddToImportsToRelatedDocuments(

src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/DocumentStateTest.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ public void DocumentState_WithProjectChange_CachesSnapshotText()
7070
.WithText(_text, VersionStamp.Create());
7171

7272
// Act
73-
var state = original.WithProjectChange();
73+
var state = original.WithProjectChange(cacheComputedState: false);
7474

7575
// Assert
7676
Assert.True(state.TryGetText(out _));
@@ -87,7 +87,7 @@ public async Task DocumentState_WithProjectChange_CachesLoadedText()
8787
await original.GetTextAsync(DisposalToken);
8888

8989
// Act
90-
var state = original.WithProjectChange();
90+
var state = original.WithProjectChange(cacheComputedState: false);
9191

9292
// Assert
9393
Assert.True(state.TryGetText(out _));

src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/ProjectStateTest.cs

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_ConfigurationChange_Up
558558
Assert.Same(originalTagHelpers[i], actualTagHelpers[i]);
559559
}
560560

561+
Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
561562
Assert.NotEqual(originalProjectWorkspaceStateVersion, actualProjectWorkspaceStateVersion);
562563

563564
Assert.NotSame(original.Documents[_documents[1].FilePath], state.Documents[_documents[1].FilePath]);
@@ -584,6 +585,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_RootNamespaceChange_Up
584585

585586
// Assert
586587
Assert.NotSame(original, state);
588+
Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
587589
}
588590

589591
[Fact]
@@ -602,6 +604,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_NoConfigurationChange_
602604

603605
// Assert
604606
Assert.Same(original, state);
607+
Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
605608
}
606609

607610
[Fact]
@@ -623,6 +626,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_CallsConfigurationChan
623626
// Assert
624627
Assert.NotEqual(original.Version, state.Version);
625628
Assert.Same(_hostProjectWithConfigurationChange, state.HostProject);
629+
Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
626630
Assert.Equal(2, callCount);
627631
}
628632

@@ -640,6 +644,38 @@ public void ProjectState_WithHostProjectAndWorkspaceState_ResetsImportedDocument
640644
var importMap = Assert.Single(state.ImportsToRelatedDocuments);
641645
var documentFilePath = Assert.Single(importMap.Value);
642646
Assert.Equal(TestProjectData.SomeProjectFile1.FilePath, documentFilePath);
647+
Assert.NotSame(original.ImportsToRelatedDocuments, state.ImportsToRelatedDocuments);
648+
Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
649+
}
650+
651+
[Fact]
652+
public void ProjectState_WithHostProjectAndWorkspaceState_ProjectWorkspaceStateChange_CachesImportedDocuments()
653+
{
654+
// Arrange
655+
var original = ProjectState.Create(ProjectEngineFactoryProvider, LanguageServerFeatureOptions, _hostProject, _projectWorkspaceState);
656+
original = original.WithAddedHostDocument(TestProjectData.SomeProjectFile1, DocumentState.EmptyLoader);
657+
658+
var changed = ProjectWorkspaceState.Default;
659+
660+
// Act
661+
var state = original.WithHostProjectAndWorkspaceState(_hostProject, changed);
662+
663+
// Assert
664+
Assert.Same(original.ImportsToRelatedDocuments, state.ImportsToRelatedDocuments);
665+
}
666+
667+
[Fact]
668+
public void ProjectState_WithHostProjectAndWorkspaceState_HostProjectChange_DoesntCacheImportedDocuments()
669+
{
670+
// Arrange
671+
var original = ProjectState.Create(ProjectEngineFactoryProvider, LanguageServerFeatureOptions, _hostProject, _projectWorkspaceState);
672+
original = original.WithAddedHostDocument(TestProjectData.SomeProjectFile1, DocumentState.EmptyLoader);
673+
674+
// Act
675+
var state = original.WithHostProjectAndWorkspaceState(_hostProjectWithConfigurationChange, _projectWorkspaceState);
676+
677+
// Assert
678+
Assert.NotSame(original.ImportsToRelatedDocuments, state.ImportsToRelatedDocuments);
643679
}
644680

645681
[Fact]
@@ -679,6 +715,8 @@ public void ProjectState_WithHostProjectAndWorkspaceState_Changed()
679715

680716
Assert.NotSame(original.Documents[_documents[1].FilePath], state.Documents[_documents[1].FilePath]);
681717
Assert.NotSame(original.Documents[_documents[2].FilePath], state.Documents[_documents[2].FilePath]);
718+
719+
Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
682720
}
683721

684722
[Fact]
@@ -713,6 +751,8 @@ public void ProjectState_WithHostProjectAndWorkspaceState_Changed_TagHelpersChan
713751

714752
Assert.NotSame(original.Documents[_documents[1].FilePath], state.Documents[_documents[1].FilePath]);
715753
Assert.NotSame(original.Documents[_documents[2].FilePath], state.Documents[_documents[2].FilePath]);
754+
755+
Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
716756
}
717757

718758
[Fact]
@@ -756,6 +796,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_CallsWorkspaceProjectC
756796

757797
// Assert
758798
Assert.NotEqual(original.Version, state.Version);
799+
Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
759800
Assert.Equal(2, callCount);
760801
}
761802

@@ -1034,10 +1075,10 @@ public override DocumentState WithTextLoader(TextLoader loader)
10341075
return base.WithTextLoader(loader);
10351076
}
10361077

1037-
public override DocumentState WithProjectChange()
1078+
public override DocumentState WithProjectChange(bool cacheComputedState)
10381079
{
10391080
_onConfigurationChange?.Invoke();
1040-
return base.WithProjectChange();
1081+
return base.WithProjectChange(cacheComputedState);
10411082
}
10421083

10431084
public override DocumentState WithImportsChange()

0 commit comments

Comments
 (0)