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

Fine tuning of what types of project update affect what state #11213

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Fine tuning of what types of project update affect what state #11213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2740929
Don't overbuild when only project workspace state has changed
davidwengier Nov 14, 2024
39e8d1f
PR Feedback
davidwengier Nov 14, 2024
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: 5 additions & 3 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,14 @@ public virtual DocumentState WithImportsChange()
return state;
}

public virtual DocumentState WithProjectChange()
public virtual DocumentState WithProjectChange(bool cacheComputedState)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not sure when or why this should be true or false. I see in the PR but not really the impact of it

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll admit this isn't great, and I think the reasoning around what the computed state actually achieves, and whether its worth caching, is not fully understood by anyone. Or perhaps better said, I don't think I, or anyone else, could write a convincing comment explaining why this value should be true or false. I've made things match previous behaviour, and that seems to have had no bad outcomes, and I look forward to Dustin removing the computed state tracker in the future.

{
var state = new DocumentState(HostDocument, Version + 1, _textAndVersion, _textLoader);

// Optimistically cache the computed state
state._computedState = new ComputedStateTracker(_computedState);
if (cacheComputedState)
{
state._computedState = new ComputedStateTracker(_computedState);
}

return state;
}
Expand Down
32 changes: 21 additions & 11 deletions src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ProjectState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,26 +274,36 @@ public ProjectState WithChangedHostDocument(HostDocument hostDocument, TextLoade

public ProjectState WithHostProjectAndWorkspaceState(HostProject hostProject, ProjectWorkspaceState projectWorkspaceState)
{
if (HostProject.Configuration.Equals(hostProject.Configuration) &&
HostProject.RootNamespace == hostProject.RootNamespace &&
ProjectWorkspaceState.Equals(projectWorkspaceState))
var configUnchanged = (HostProject.Configuration.Equals(hostProject.Configuration) &&
HostProject.RootNamespace == hostProject.RootNamespace);

if (configUnchanged && ProjectWorkspaceState.Equals(projectWorkspaceState))
{
return this;
}

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

// If the host project has changed then we need to recompute the imports map
var importsToRelatedDocuments = s_emptyImportsToRelatedDocuments;
var importsToRelatedDocuments = configUnchanged
? ImportsToRelatedDocuments
: ComputeImportsToRelatedDocuments(documents);

var state = new ProjectState(this, numberOfDocumentsMayHaveChanged: !configUnchanged, hostProject, projectWorkspaceState, documents, importsToRelatedDocuments);
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit hidden, so I'll call it out here. Where I said in the PR description that updating the DocumentCollectionVersion was the main issue, its this numberOfDocumentsMayHaveChanged parameter.

In my original PR I equated any project change to a possible document change, but tag helpers changing is definitely not that.

return state;

foreach (var document in documents)
ImmutableDictionary<string, ImmutableArray<string>> ComputeImportsToRelatedDocuments(ImmutableDictionary<string, DocumentState> documents)
{
var importTargetPaths = GetImportDocumentTargetPaths(document.Value.HostDocument);
importsToRelatedDocuments = AddToImportsToRelatedDocuments(importsToRelatedDocuments, document.Value.HostDocument.FilePath, importTargetPaths);
}
var importsToRelatedDocuments = s_emptyImportsToRelatedDocuments;

var state = new ProjectState(this, numberOfDocumentsMayHaveChanged: true, hostProject, projectWorkspaceState, documents, importsToRelatedDocuments);
return state;
foreach (var document in documents)
{
var importTargetPaths = GetImportDocumentTargetPaths(document.Value.HostDocument);
importsToRelatedDocuments = AddToImportsToRelatedDocuments(importsToRelatedDocuments, document.Value.HostDocument.FilePath, importTargetPaths);
}

return importsToRelatedDocuments;
}
}

internal static ImmutableDictionary<string, ImmutableArray<string>> AddToImportsToRelatedDocuments(
Expand Down
4 changes: 2 additions & 2 deletions ...azor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/DocumentStateTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void DocumentState_WithProjectChange_CachesSnapshotText()
.WithText(_text, VersionStamp.Create());

// Act
var state = original.WithProjectChange();
var state = original.WithProjectChange(cacheComputedState: false);

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

// Act
var state = original.WithProjectChange();
var state = original.WithProjectChange(cacheComputedState: false);

// Assert
Assert.True(state.TryGetText(out _));
Expand Down
45 changes: 43 additions & 2 deletions ...Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/ProjectStateTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_ConfigurationChange_Up
Assert.Same(originalTagHelpers[i], actualTagHelpers[i]);
}

Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
Assert.NotEqual(originalProjectWorkspaceStateVersion, actualProjectWorkspaceStateVersion);

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

// Assert
Assert.NotSame(original, state);
Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
}

[Fact]
Expand All @@ -602,6 +604,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_NoConfigurationChange_

// Assert
Assert.Same(original, state);
Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
}

[Fact]
Expand All @@ -623,6 +626,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_CallsConfigurationChan
// Assert
Assert.NotEqual(original.Version, state.Version);
Assert.Same(_hostProjectWithConfigurationChange, state.HostProject);
Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
Assert.Equal(2, callCount);
}

Expand All @@ -640,6 +644,38 @@ public void ProjectState_WithHostProjectAndWorkspaceState_ResetsImportedDocument
var importMap = Assert.Single(state.ImportsToRelatedDocuments);
var documentFilePath = Assert.Single(importMap.Value);
Assert.Equal(TestProjectData.SomeProjectFile1.FilePath, documentFilePath);
Assert.NotSame(original.ImportsToRelatedDocuments, state.ImportsToRelatedDocuments);
Assert.NotEqual(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
}

[Fact]
public void ProjectState_WithHostProjectAndWorkspaceState_ProjectWorkspaceStateChange_CachesImportedDocuments()
{
// Arrange
var original = ProjectState.Create(ProjectEngineFactoryProvider, LanguageServerFeatureOptions, _hostProject, _projectWorkspaceState);
original = original.WithAddedHostDocument(TestProjectData.SomeProjectFile1, DocumentState.EmptyLoader);

var changed = ProjectWorkspaceState.Default;

// Act
var state = original.WithHostProjectAndWorkspaceState(_hostProject, changed);

// Assert
Assert.Same(original.ImportsToRelatedDocuments, state.ImportsToRelatedDocuments);
}

[Fact]
public void ProjectState_WithHostProjectAndWorkspaceState_HostProjectChange_DoesntCacheImportedDocuments()
{
// Arrange
var original = ProjectState.Create(ProjectEngineFactoryProvider, LanguageServerFeatureOptions, _hostProject, _projectWorkspaceState);
original = original.WithAddedHostDocument(TestProjectData.SomeProjectFile1, DocumentState.EmptyLoader);

// Act
var state = original.WithHostProjectAndWorkspaceState(_hostProjectWithConfigurationChange, _projectWorkspaceState);

// Assert
Assert.NotSame(original.ImportsToRelatedDocuments, state.ImportsToRelatedDocuments);
}

[Fact]
Expand Down Expand Up @@ -679,6 +715,8 @@ public void ProjectState_WithHostProjectAndWorkspaceState_Changed()

Assert.NotSame(original.Documents[_documents[1].FilePath], state.Documents[_documents[1].FilePath]);
Assert.NotSame(original.Documents[_documents[2].FilePath], state.Documents[_documents[2].FilePath]);

Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
}

[Fact]
Expand Down Expand Up @@ -713,6 +751,8 @@ public void ProjectState_WithHostProjectAndWorkspaceState_Changed_TagHelpersChan

Assert.NotSame(original.Documents[_documents[1].FilePath], state.Documents[_documents[1].FilePath]);
Assert.NotSame(original.Documents[_documents[2].FilePath], state.Documents[_documents[2].FilePath]);

Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
}

[Fact]
Expand Down Expand Up @@ -756,6 +796,7 @@ public void ProjectState_WithHostProjectAndWorkspaceState_CallsWorkspaceProjectC

// Assert
Assert.NotEqual(original.Version, state.Version);
Assert.Equal(original.DocumentCollectionVersion, state.DocumentCollectionVersion);
Assert.Equal(2, callCount);
}

Expand Down Expand Up @@ -1034,10 +1075,10 @@ public override DocumentState WithTextLoader(TextLoader loader)
return base.WithTextLoader(loader);
}

public override DocumentState WithProjectChange()
public override DocumentState WithProjectChange(bool cacheComputedState)
{
_onConfigurationChange?.Invoke();
return base.WithProjectChange();
return base.WithProjectChange(cacheComputedState);
}

public override DocumentState WithImportsChange()
Expand Down