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.

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

Bulk update project when several documents change #55813

Merged
merged 3 commits into from
Aug 25, 2021

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Aug 23, 2021

When changing the active solution configuration, all the documents in the solution change making for a very slow incremental update. This change detects "large" changes within a project as those touching three or more documents in a single operation, and ensures the assets are synchronized in bulk for this case.

Total StreamJsonRpc time prior to this change is 18.4 seconds in response to a configuration change (3138 GetAssetsAsync operations). Total time after this change is 6.5 seconds (1201 GetAssetsAsync operations).

Fixes AB#1365014

@sharwell sharwell requested a review from a team as a code owner August 23, 2021 15:23
@@ -356,6 +361,14 @@ private async Task<Project> UpdateProjectInfoAsync(Project project, Checksum inf
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.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

lgtm, would just like to know if there's a test somewhere that covers this.

@sharwell sharwell merged commit ef8ab06 into dotnet:main Aug 25, 2021
@ghost ghost added this to the Next milestone Aug 25, 2021
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
@sharwell sharwell deleted the bulk-update branch January 13, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants