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

Bulk update project when several documents change #55813

Merged
merged 3 commits into from
Aug 25, 2021
Merged

Bulk update project when several documents change #55813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cb92ee
Bulk update project when several documents change
sharwell Aug 23, 2021
787530b
Add debug assertion to clarify code flow
sharwell Aug 23, 2021
93c7e4b
Add a test for batch synchronizing projects
sharwell Aug 25, 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
87 changes: 71 additions & 16 deletions src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,10 @@ await solution.State.GetChecksumAsync(CancellationToken.None),
await remoteWorkspace.CurrentSolution.State.GetChecksumAsync(CancellationToken.None));
}

[Fact]
public async Task TestRemoteHostSynchronizeIncrementalUpdate()
[Theory]
[CombinatorialData]
[WorkItem(1365014, "https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1365014")]
public async Task TestRemoteHostSynchronizeIncrementalUpdate(bool applyInBatch)
{
using var workspace = CreateWorkspace();

Expand All @@ -301,14 +303,14 @@ await solution.State.GetChecksumAsync(CancellationToken.None),
await remoteWorkspace.CurrentSolution.State.GetChecksumAsync(CancellationToken.None));

// incrementally update
solution = await VerifyIncrementalUpdatesAsync(remoteWorkspace, client, solution, csAddition: " ", vbAddition: " ");
solution = await VerifyIncrementalUpdatesAsync(remoteWorkspace, client, solution, applyInBatch, csAddition: " ", vbAddition: " ");

Assert.Equal(
await solution.State.GetChecksumAsync(CancellationToken.None),
await remoteWorkspace.CurrentSolution.State.GetChecksumAsync(CancellationToken.None));

// incrementally update
solution = await VerifyIncrementalUpdatesAsync(remoteWorkspace, client, solution, csAddition: "\r\nclass Addition { }", vbAddition: "\r\nClass VB\r\nEnd Class");
solution = await VerifyIncrementalUpdatesAsync(remoteWorkspace, client, solution, applyInBatch, csAddition: "\r\nclass Addition { }", vbAddition: "\r\nClass VB\r\nEnd Class");

Assert.Equal(
await solution.State.GetChecksumAsync(CancellationToken.None),
Expand Down Expand Up @@ -349,7 +351,7 @@ public void TestRemoteWorkspaceCircularReferences()
Assert.NotNull(solution);
}

private async Task<Solution> VerifyIncrementalUpdatesAsync(Workspace remoteWorkspace, RemoteHostClient client, Solution solution, string csAddition, string vbAddition)
private async Task<Solution> VerifyIncrementalUpdatesAsync(Workspace remoteWorkspace, RemoteHostClient client, Solution solution, bool applyInBatch, string csAddition, string vbAddition)
{
var remoteSolution = remoteWorkspace.CurrentSolution;
var projectIds = solution.ProjectIds;
Expand All @@ -358,19 +360,39 @@ private async Task<Solution> VerifyIncrementalUpdatesAsync(Workspace remoteWorks
{
var projectName = $"Project{i}";
var project = solution.GetProject(projectIds[i]);
var changedDocuments = new List<string>();

var documentIds = project.DocumentIds;
for (var j = 0; j < documentIds.Count; j++)
{
var documentName = $"Document{j}";

var currentSolution = UpdateSolution(solution, projectName, documentName, csAddition, vbAddition);
await UpdatePrimaryWorkspace(client, currentSolution);
changedDocuments.Add(documentName);

solution = currentSolution;

if (!applyInBatch)
{
await UpdateAndVerifyAsync();
}
}

if (applyInBatch)
{
await UpdateAndVerifyAsync();
}

async Task UpdateAndVerifyAsync()
{
var documentNames = changedDocuments.ToImmutableArray();
changedDocuments.Clear();

await UpdatePrimaryWorkspace(client, solution);

var currentRemoteSolution = remoteWorkspace.CurrentSolution;
VerifyStates(remoteSolution, currentRemoteSolution, projectName, documentName);
VerifyStates(remoteSolution, currentRemoteSolution, projectName, documentNames);

solution = currentSolution;
remoteSolution = currentRemoteSolution;

Assert.Equal(
Expand All @@ -382,17 +404,17 @@ await solution.State.GetChecksumAsync(CancellationToken.None),
return solution;
}

private static void VerifyStates(Solution solution1, Solution solution2, string projectName, string documentName)
private static void VerifyStates(Solution solution1, Solution solution2, string projectName, ImmutableArray<string> documentNames)
{
Assert.True(solution1.Workspace is RemoteWorkspace);
Assert.True(solution2.Workspace is RemoteWorkspace);

SetEqual(solution1.ProjectIds, solution2.ProjectIds);

var (project, document) = GetProjectAndDocument(solution1, projectName, documentName);
var (project, documents) = GetProjectAndDocuments(solution1, projectName, documentNames);

var projectId = project.Id;
var documentId = document.Id;
var documentIds = documents.SelectAsArray(document => document.Id);

var projectIds = solution1.ProjectIds;
for (var i = 0; i < projectIds.Count; i++)
Expand All @@ -406,12 +428,12 @@ private static void VerifyStates(Solution solution1, Solution solution2, string
{
SetEqual(solution1.GetProject(currentProjectId).DocumentIds, solution2.GetProject(currentProjectId).DocumentIds);

var documentIds = solution1.GetProject(currentProjectId).DocumentIds;
for (var j = 0; j < documentIds.Count; j++)
var documentIdsInProject = solution1.GetProject(currentProjectId).DocumentIds;
for (var j = 0; j < documentIdsInProject.Count; j++)
{
var currentDocumentId = documentIds[j];
var currentDocumentId = documentIdsInProject[j];

var documentStateShouldSame = documentId != currentDocumentId;
var documentStateShouldSame = !documentIds.Contains(currentDocumentId);
Assert.Equal(documentStateShouldSame, object.ReferenceEquals(solution1.GetDocument(currentDocumentId).State, solution2.GetDocument(currentDocumentId).State));
}
}
Expand Down Expand Up @@ -444,14 +466,23 @@ private static SourceText GetNewText(Document document, string csAddition, strin
return SourceText.From(document.State.GetTextSynchronously(CancellationToken.None).ToString() + vbAddition);
}

private static (Project, Document) GetProjectAndDocument(Solution solution, string projectName, string documentName)
private static (Project project, Document document) GetProjectAndDocument(Solution solution, string projectName, string documentName)
{
var project = solution.Projects.First(p => string.Equals(p.Name, projectName, StringComparison.OrdinalIgnoreCase));
var document = project.Documents.First(d => string.Equals(d.Name, documentName, StringComparison.OrdinalIgnoreCase));

return (project, document);
}

private static (Project project, ImmutableArray<Document> documents) GetProjectAndDocuments(Solution solution, string projectName, ImmutableArray<string> documentNames)
{
var project = solution.Projects.First(p => string.Equals(p.Name, projectName, StringComparison.OrdinalIgnoreCase));
var documents = documentNames.SelectAsArray(
documentName => project.Documents.First(d => string.Equals(d.Name, documentName, StringComparison.OrdinalIgnoreCase)));

return (project, documents);
}

// make sure we always move remote workspace forward
private int _solutionVersion = 0;

Expand Down Expand Up @@ -502,6 +533,30 @@ private static Solution Populate(Solution solution)
"cs additional file content2"
}, Array.Empty<ProjectId>());

solution = AddProject(solution, LanguageNames.CSharp, new[]
{
"class CS { }",
"class CS2 { }",
"class CS3 { }",
"class CS4 { }",
"class CS5 { }",
}, new[]
{
"cs additional file content"
}, Array.Empty<ProjectId>());

solution = AddProject(solution, LanguageNames.VisualBasic, new[]
{
"Class VB\r\nEnd Class",
"Class VB2\r\nEnd Class",
"Class VB3\r\nEnd Class",
"Class VB4\r\nEnd Class",
"Class VB5\r\nEnd Class",
}, new[]
{
"vb additional file content"
}, Array.Empty<ProjectId>());

return solution;
}

Expand Down
15 changes: 15 additions & 0 deletions src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.SolutionCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -242,6 +243,7 @@ await _assetProvider.GetAssetAsync<CompilationOptions>(
{
project = await UpdateDocumentsAsync(
project,
newProjectChecksums,
project.State.DocumentStates.States.Values,
oldProjectChecksums.Documents,
newProjectChecksums.Documents,
Expand All @@ -254,6 +256,7 @@ await _assetProvider.GetAssetAsync<CompilationOptions>(
{
project = await UpdateDocumentsAsync(
project,
newProjectChecksums,
project.State.AdditionalDocumentStates.States.Values,
oldProjectChecksums.AdditionalDocuments,
newProjectChecksums.AdditionalDocuments,
Expand All @@ -266,6 +269,7 @@ await _assetProvider.GetAssetAsync<CompilationOptions>(
{
project = await UpdateDocumentsAsync(
project,
newProjectChecksums,
project.State.AnalyzerConfigDocumentStates.States.Values,
oldProjectChecksums.AnalyzerConfigDocuments,
newProjectChecksums.AnalyzerConfigDocuments,
Expand Down Expand Up @@ -337,6 +341,7 @@ private async Task<Project> UpdateProjectInfoAsync(Project project, Checksum inf

private async Task<Project> UpdateDocumentsAsync(
Project project,
ProjectStateChecksums projectChecksums,
IEnumerable<TextDocumentState> existingTextDocumentStates,
ChecksumCollection oldChecksums,
ChecksumCollection newChecksums,
Expand All @@ -356,6 +361,14 @@ private async Task<Project> UpdateDocumentsAsync(
var oldMap = await GetDocumentMapAsync(existingTextDocumentStates, olds.Object).ConfigureAwait(false);
var newMap = await GetDocumentMapAsync(_assetProvider, news.Object).ConfigureAwait(false);

// If more than two documents changed during a single update, perform a bulk synchronization on the
Copy link
Member

Choose a reason for hiding this comment

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

looks fine with me. do we have any explicit tests where we resync with multiple files changing just to ensure that nothing breaks with this?

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 try to make one

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ A test is now added. I verified under debugger that the test goes through the new path.

// project to avoid large numbers of small synchronization calls during document updates.
// 🔗 https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1365014
if (newMap.Count > 2)
{
await _assetProvider.SynchronizeProjectAssetsAsync(new[] { projectChecksums.Checksum }, _cancellationToken).ConfigureAwait(false);
}

// added document
ImmutableArray<DocumentInfo>.Builder? lazyDocumentsToAdd = null;
foreach (var (documentId, newDocumentChecksums) in newMap)
Expand Down Expand Up @@ -467,6 +480,8 @@ private async Task<Dictionary<DocumentId, DocumentStateChecksums>> GetDocumentMa

foreach (var kv in documentChecksums)
{
Debug.Assert(assetProvider.EnsureCacheEntryIfExists(kv.Item2.Info), "Expected the prior call to GetAssetsAsync to obtain all items for this loop.");

var info = await assetProvider.GetAssetAsync<DocumentInfo.DocumentAttributes>(kv.Item2.Info, _cancellationToken).ConfigureAwait(false);
map.Add(info.Id, kv.Item2);
}
Expand Down