Collapse Reset then Add to Update in project state synchronizer #10330

davidwengier · 2024-04-30T02:19:45Z

Fixes #10306
Alternative to #10308

This change keeps the AsyncBatchingWorkQueue in the project state synchronizer, but still collapses a Reset then Add to be an Update, so that we don't migrate every document onto the misc files project, and then back on to the real project, every time a new component is added.

davidwengier · 2024-04-30T03:25:25Z

...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs

-                                Configuration file path: '{configurationFilePath}'
-                                """);
-                        }
+                        var projectKey = ProjectKey.FromString(FilePathNormalizer.GetNormalizedDirectoryName(args.ConfigurationFilePath));


Could do a ProjectKey.From(ProjectConfigurationFileChangeEventArgs) overload?

It seems a bit cumbersome to make the ProjectKey API dependent on ProjectConfigurationFileChangeEventArgs. I'd rather see a ProjectConfigurationFileChangeEventArgs.ToProjectKey() method.

Good idea, I should have thought of that!

davidwengier · 2024-04-30T03:26:16Z

.../Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs

 {
    [Fact]
-    public async Task ProjectConfigurationFileChanged_Removed_UnknownDocumentNoops()
+    public async Task ProjectConfigurationFileChanged_Removed_UntrackedProject_CallsUpdate()


This is a change to the logic of the synchronizer itself, where it will call Update, but Update will no-op for a non-existent project, so holistically the behaviour is the same

davidwengier · 2024-04-30T03:26:26Z

.../Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs


    [Fact]
-    public async Task ProjectConfigurationFileChanged_Changed_UntrackedProject_Noops()
+    public async Task ProjectConfigurationFileChanged_Changed_UntrackedProject_CallsUpdate()


As above, holistically the same

davidwengier · 2024-04-30T03:28:00Z

.../Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs

-    public async Task ProjectConfigurationFileChanged_RemoveThenAdd_OnlyAdds()
+    public async Task ProjectConfigurationFileChanged_RemoveThenAdd_Updates()


This test didn't do what its name implied it did. It actually verified that if you Add then Change, you get an Add then Update.

The test now does what its name implies, which is actually testing the restored functionality of collapsing removes and adds into a single Update operation.

Thank you! These tests made my eyes cross. 😄

DustinCampbell

I definitely like this approach better than my PR!

...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs

DustinCampbell · 2024-04-30T15:52:20Z

...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs

+        // and then we can process the most recent unique items as normal
+        for (var i = _indicesToSkip.Count - 1; i >= 0; i--)
+        {
+            itemsToProcess.RemoveAt(_indicesToSkip[i]);


❔Can we just skip indices in the loop below? That would avoid all of the array copying here.

The problem is that we want to skip indicies before collapsing. So for example, if we create a new bin file, then update it, this method will see an Add, a Remove, then an Add. We need to convert the second Add to an Update, and skip index 1. If we do that in the loop that calls GetMostRecentUniqueItems though, we'll be actually processing a list that contains Remove then Add, because the first Add will have been de-duped. Skiping index 1 there is not what we want.

Your points about copying are totally fair though. I guess I can inline the logic from GetMostRecentUniqueItems into the the loop above this, and do it all in one backwards loop through the set.

Okay this was a big BIG pain, so I haven't quite got there, but see what you think.

I did remove a hashset, and remove the most egregious array resizing that this RemoveAt method causes. I also fixed a bug in the previous code that I didn't realise I had :)

Also, and you might not like this, but I swapped from records to classes. We don't actually want value semantics for the work items, and in fact we don't get it anyway (RazorProjectInfo doesn't provide an equality comparer), and I'm not entirely convinced that the tests were even valid. They are reusing the same project info for all work items, but in the real world, for different project info instances, I think the generated GetHashCode would have returned different values, resulting in no actual de-duping at runtime, because the HashSet Contains checked wouldn't have even called the Equals method in the comparer.

...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs

DustinCampbell · 2024-04-30T15:58:30Z

...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectConfigurationStateSynchronizer.cs

-                                Configuration file path: '{configurationFilePath}'
-                                """);
-                        }
+                        var projectKey = ProjectKey.FromString(FilePathNormalizer.GetNormalizedDirectoryName(args.ConfigurationFilePath));


It seems a bit cumbersome to make the ProjectKey API dependent on ProjectConfigurationFileChangeEventArgs. I'd rather see a ProjectConfigurationFileChangeEventArgs.ToProjectKey() method.

DustinCampbell · 2024-04-30T15:59:11Z

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

    // end up. All creation logic is here in one place to ensure this is consistent.
    public static ProjectKey From(HostProject hostProject) => new(hostProject.IntermediateOutputPath);
    public static ProjectKey From(IProjectSnapshot project) => new(project.IntermediateOutputPath);
+    public static ProjectKey From(RazorProjectInfo project) => new(FilePathNormalizer.GetNormalizedDirectoryName(project.SerializedFilePath));


Hey! You found a use for SerializedFilePath! I hope it's accurate. 😄

DustinCampbell · 2024-04-30T16:00:40Z

.../Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs

-    public async Task ProjectConfigurationFileChanged_RemoveThenAdd_OnlyAdds()
+    public async Task ProjectConfigurationFileChanged_RemoveThenAdd_Updates()


Thank you! These tests made my eyes cross. 😄

DustinCampbell · 2024-04-30T16:01:10Z

.../Microsoft.AspNetCore.Razor.LanguageServer.Test/ProjectConfigurationStateSynchronizerTest.cs

+
    private TestProjectConfigurationStateSynchronizer GetSynchronizer(IRazorProjectService razorProjectService)
-        => new(razorProjectService, LoggerFactory, TestLanguageServerFeatureOptions.Instance, TimeSpan.FromMilliseconds(5));
+        => new(razorProjectService, LoggerFactory, TestLanguageServerFeatureOptions.Instance, TimeSpan.FromMilliseconds(50));


Why does this need to be 50 milliseconds? Is there a race?

Not a race that leads to a bug, but on my laptop on battery the test where I add half a dozen changed events would sometimes end up processing two batches, which means UpdateProjectAsync would be called one more time than we expect. Though to be fair, the number of changed events I'm adding is a little pathological :)

Could just directly test the process method and avoid the queue for some tests though, since the event handler code is simpler now anyway, if you prefer?

…#10335) Fixes #10306 This change takes a different approach than #10308 and #10330 and #10332 because life was meant to be interesting. The original problem we were trying to solve is that when a Reset then Add came in, we would migrate all of the documents out of the project, then migrate them all back in again. The various solutions in the other PRs you can read about at your leisure, this one takes a rather simple approach: 1. Ignore everything but the most recent update for a specific project 2. If we need to update a project, but it doesn't exist, add it In the long run we should remove `IRazorProjectService.AddProjectAsync` entirely, and rename `IRazorProjectService.UpdateProjectAsync` to `ResetProjectAsync`, but doing that now would just conflict with other open PRs.

Collapse Reset then Add to Update in project state synchronizer

130eeec

davidwengier requested a review from a team as a code owner April 30, 2024 02:19

davidwengier commented Apr 30, 2024

View reviewed changes

davidwengier mentioned this pull request Apr 30, 2024

Don't use batching work queue in ProjectConfigurationStateSynchronizer #10308

Closed

Add more tests, and fix a bug found by the tests that were added

90b56bd

DustinCampbell reviewed Apr 30, 2024

View reviewed changes

PR Feedback

0d46788

DustinCampbell mentioned this pull request May 1, 2024

YAPCSSF: Yet Another ProjectConfigurationStateSynchronizer fix #10332

Closed

davidwengier mentioned this pull request May 2, 2024

YAPCSSF: Automatically add projects if they don't exist when updating #10335

Merged

davidwengier closed this May 2, 2024

davidwengier deleted the dev/dawengie/ProjectStateSynchronizer branch May 2, 2024 23:12

		public async Task ProjectConfigurationFileChanged_RemoveThenAdd_OnlyAdds()
		public async Task ProjectConfigurationFileChanged_RemoveThenAdd_Updates()

Collapse Reset then Add to Update in project state synchronizer #10330

Collapse Reset then Add to Update in project state synchronizer #10330

Uh oh!

Conversation

davidwengier commented Apr 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidwengier Apr 30, 2024 •

edited

Loading

davidwengier Apr 30, 2024 •

edited

Loading