Reduce IVsFreeThreadedFileChangeEvents2.DirectoryChangedEx2 notifications from VS shell #75815

ToddGrun · 2024-11-08T00:17:54Z

While looking into another FileWatcher issue, I noticed that we were getting more notifications from shell on directory changes than I would have expected. It turns out this is because we don't currently combine WatchDirectory operations as we do WatchFile/UnwatchFile/UnwatchDirectories.

This is generally the desired behavior as vs shell doesn't support multiple directories being supported by a single notification. However, they do support multiple filters on the same directory, and this is exactly the case that we usualy experience when processing a batch of WatcherOperations. This PR simply allows combining of directory watch notifications for the same directory (and sink), keeping track of the combined set of filters that all requests had.

Note that this PR also changed the mergability of WatchFiles by only allowing merging if the sinks are the same. I don't have context to be completely confident the prior behavior is a bug, but it sure seems wrong.

…ions from VS shell While looking into another FileWatcher issue, I noticed that we were getting more notifications from shell on directory changes than I would have expected. It turns out this is because we don't currently combine WatchDirectory operations as we do WatchFile/UnwatchFile/UnwatchDirectories. This is generally the desired behavior as vs shell doesn't support multiple directories being supported by a single notification. However, they do support multiple filters on the same directory, and this is exactly the case that we usualy experience when processing a batch of WatcherOperations. This PR simply allows combining of directory watch notifications for the same directory (and sink), keeping track of the combined set of filters that all requests had. Note that this PR also changed the mergability of WatchFiles by only allowing merging if the sinks are the same. I don't have context to be completely confident the prior behavior is a bug, but it sure seems wrong.

ToddGrun · 2024-11-08T16:15:19Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

-            return (_kind == other._kind);
+            if (_kind == Kind.WatchFiles)
+            {
+                return other._sink == _sink;


@jasonmalinowski -- This is the code that seemed like a bug before. I definitely see WatchFiles requests with different sinks getting merged.

Looks like this is fixing #74716 then.

CyrusNajmabadi · 2024-11-08T17:33:49Z

In this PR, or in a followup, can you examine if TimeSpan.Zero is really a good idea for the ABWQ delay. As this is IO and FS stuff, i really think it would make more sense to have a more reasonable delay (like 500ms or a second). From user perspective, they'll never notice this. But it means we can collect a tremendous amount more, and process and analyze the results in much larger batches for things like deduping and whatnot. TimeSpan.Zero is really for cases of "we are both going to getting an insane firehose of messages AND we must issue them immediately or else badness will occur". For disk notifications, the latter just doesn't apply here. No one will be harmed AFAICT if we wait 500ms.

… come in bursts.

ToddGrun · 2024-11-08T19:42:22Z

In this PR, or in a followup, can you examine if TimeSpan.Zero is really a good idea for the ABWQ delay. As this is IO and FS stuff, i really think it would make more sense to have a more reasonable delay (like 500ms or a second). From user perspective, they'll never notice this. But it means we can collect a tremendous amount more, and process and analyze the results in much larger batches for things like deduping and whatnot. TimeSpan.Zero is really for cases of "we are both going to getting an insane firehose of messages AND we must issue them immediately or else badness will occur". For disk notifications, the latter just doesn't apply here. No one will be harmed AFAICT if we wait 500ms.

From local testing of open/close roslyn sln, I saw the following:

0 ms delay
2046 calls to ProcessBatchAsync
19942 total operations passed in
8502 merged batches created

100 ms
47 calls to ProcessBatchAsync
19942 total operations passed in
6972 merged batches created

500 ms
18 calls to ProcessBatchAsync
19942 total operations passed in
6518 merged batches created

CyrusNajmabadi · 2024-11-08T20:04:12Z

Love it. This is also nice as we really do want to "back off" when notifications come in. They often come in in huge flurries, and we don't want to then be contending with the disk while we're being notified about chnages from other files. 500ms is a nice compromise.

jasonmalinowski

I believe this breaks how the cookies have to get back to the original context for the directory watch, so later unsubscribes won't unsubscribe. I could be wrong, but we should really have a test either way that two directory watches combine, and you can successfully remove them later.

jasonmalinowski · 2024-11-08T21:16:33Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

-        // 📝 Empirical testing during high activity (e.g. solution close) showed strong batching performance even
-        // though the batching delay is 0.
+        // 📝 Empirical testing during high activity (e.g. solution open/close) showed strong batching performance
+        // with batching delay of 500 ms.


I have no particular opinion what this number should be, but do we have some way to actually differentiate between zero and 500 ms for which is better here? If both numbers were "showing strong batching performance" how did we come up with this change?

Just to make sure, you saw the earlier comment I made regarding the different performance characteristics I saw when trying out various values?

I did not! Maybe we should add that data as a comment to the code.

jasonmalinowski · 2024-11-08T21:23:15Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

-            return (_kind == other._kind);
+            if (_kind == Kind.WatchFiles)
+            {
+                return other._sink == _sink;


It might be easier/prudent just to compare the _sink regardless of the underlying type; that way we don't ever forget to update this check and combine a different kind of operation by accident.

jasonmalinowski · 2024-11-08T21:30:08Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

+                        for (var i = 0; i < op._filters.Length; i++)
+                            filtersBuilder.Add(op._filters[i]);
+
+                        cookiesBuilder.AddRange(op._cookies);


So I think is going to break things -- the list of cookies in the WatchDirectory case isn't just the list that can get copied around, but it's actually the list that gets filled back in. Put another way, the List<uint> object is held by the Context object, and is expected that exact instance gets passed around to the ApplyAsync method so it can be filled in later. That way that same list object gets passed to the UnwatchDirectories later.

We really should add some tests here. Specifically, have a test that merging two directories and then unsubscribing them successfully results in no watches once we're done. I expect that to be broken.

@svick's test in #74716 is a pretty good start for the general pattern. Rather than firing events and seeing what's fired, it might be easier to just assert the number of watches on the underlying mocked file change service. (Although making sure events actually fire right wouldn't hurt either.)

Backed out the watch directory merging, wasn't going to be easy to solve and found a different way to get some watcher reductions.

jasonmalinowski · 2024-11-08T21:40:42Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

+                case Kind.WatchDirectory:
+                    filtersBuilder.RemoveDuplicates();
+                    return WatchDirectory(firstOp._paths[0], filtersBuilder.ToImmutable(), firstOp._sink, cookiesBuilder.ToList());
+                case Kind.UnwatchDirectories:


There's probably another problem here too: if two watches against a directory are for different filters, and we combine them, what happens on unsubscribe? Do we still keep the other one around?

Backed out the watch directory merging, wasn't going to be easy to solve and found a different way to get some watcher reductions.

…issues with that approach. However, we can still improve batching by changing the WatchDirectory callers to create fewer WatchedDirectory objects that instead specify multiple filters each.

ToddGrun · 2024-11-09T00:43:36Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

@@ -367,8 +361,13 @@ public Context(FileChangeWatcher fileChangeWatcher, ImmutableArray<WatchedDirect

            foreach (var watchedDirectory in watchedDirectories)
            {
-                _fileChangeWatcher._taskQueue.AddWork(watchedDirectories.Select(
-                    watchedDirectory => WatcherOperation.WatchDirectory(watchedDirectory.Path, watchedDirectory.ExtensionFilter, this, _directoryWatchCookies)));
+                var item = WatcherOperation.WatchDirectory(


var item = WatcherOperation.WatchDirectory(

Noticed there was a bug here in that every watched directory caused an operation to be created for the whole immutable array, not just for itself.

ToddGrun · 2024-11-09T00:45:12Z

Backed out the watch directory merging, wasn't going to be easy to solve and found a different way to get some watcher reductions.

In reply to: 2424933376

CyrusNajmabadi · 2024-11-09T02:53:57Z

src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/LspFileChangeWatcherTests.cs

@@ -61,7 +62,7 @@ public async Task CreatingDirectoryWatchRequestsDirectoryWatch()
        var tempDirectory = tempRoot.CreateDirectory();

        // Try creating a context and ensure we created the registration
-        var context = lspFileChangeWatcher.CreateContext([new ProjectSystem.WatchedDirectory(tempDirectory.Path, extensionFilter: null)]);
+        var context = lspFileChangeWatcher.CreateContext([new ProjectSystem.WatchedDirectory(tempDirectory.Path, extensionFilters: ImmutableArray<string>.Empty)]);


Suggested change

var context = lspFileChangeWatcher.CreateContext([new ProjectSystem.WatchedDirectory(tempDirectory.Path, extensionFilters: ImmutableArray<string>.Empty)]);

var context = lspFileChangeWatcher.CreateContext([new ProjectSystem.WatchedDirectory(tempDirectory.Path, extensionFilters: [])]);

CyrusNajmabadi · 2024-11-09T02:54:49Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

@@ -252,21 +252,15 @@ public static WatcherOperation CombineRange(ImmutableSegmentedList<WatcherOperat
                {
                    case Kind.WatchFiles:
                        for (var i = 0; i < op._paths.Count; i++)
-                        {
                            fileNamesBuilder.Add(op._paths[i]);


is this not just .AddRange (and the same for the below lines)?

op._paths is a OneOrMany, I don't think there is an override for ArrayBuilder.AddRange that takes that in.

CyrusNajmabadi · 2024-11-09T02:55:02Z

src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs

@@ -297,7 +291,7 @@ public bool CanCombineWith(in WatcherOperation other)
            if (_kind == Kind.WatchDirectory)
                return false;

-            return (_kind == other._kind);
+            return _kind == other._kind && _sink == other._sink;


consider doc'ing. up to you.

CyrusNajmabadi · 2024-11-09T02:55:18Z

src/VisualStudio/Core/Test/ProjectSystemShim/FileChangeWatcherTests.vb

+
+Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
+    <UseExportProvider>
+    Public Class FileChangeWatcherTests


Suggested change

Public Class FileChangeWatcherTests

Public NotInheritable Class FileChangeWatcherTests

ToddGrun requested a review from jasonmalinowski November 8, 2024 00:17

ToddGrun requested a review from a team as a code owner November 8, 2024 00:17

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 8, 2024

ToddGrun commented Nov 8, 2024

View reviewed changes

Change ABWQ timeout to 500 ms from 0 as the file change notifications…

f37008c

… come in bursts.

jasonmalinowski requested changes Nov 8, 2024

View reviewed changes

jasonmalinowski reviewed Nov 8, 2024

View reviewed changes

Remove batching of WatchDirectory calls as Jason pointed out several …

f8bb6cd

…issues with that approach. However, we can still improve batching by changing the WatchDirectory callers to create fewer WatchedDirectory objects that instead specify multiple filters each.

dotnet-policy-service bot added the VSCode label Nov 9, 2024

ToddGrun commented Nov 9, 2024

View reviewed changes

Add test almost directly from dotnet#74716

652181d

CyrusNajmabadi reviewed Nov 9, 2024

View reviewed changes

Minor feedback changes

f249cbf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce IVsFreeThreadedFileChangeEvents2.DirectoryChangedEx2 notifications from VS shell #75815

Reduce IVsFreeThreadedFileChangeEvents2.DirectoryChangedEx2 notifications from VS shell #75815

ToddGrun commented Nov 8, 2024

ToddGrun Nov 8, 2024

jasonmalinowski Nov 8, 2024

CyrusNajmabadi commented Nov 8, 2024

ToddGrun commented Nov 8, 2024

CyrusNajmabadi commented Nov 8, 2024

jasonmalinowski left a comment

jasonmalinowski Nov 8, 2024

ToddGrun Nov 8, 2024

jasonmalinowski Nov 9, 2024

jasonmalinowski Nov 8, 2024

jasonmalinowski Nov 8, 2024

jasonmalinowski Nov 8, 2024

ToddGrun Nov 9, 2024

jasonmalinowski Nov 8, 2024

ToddGrun Nov 9, 2024

ToddGrun Nov 9, 2024

ToddGrun commented Nov 9, 2024

CyrusNajmabadi Nov 9, 2024

CyrusNajmabadi Nov 9, 2024

ToddGrun Nov 9, 2024

CyrusNajmabadi Nov 9, 2024

CyrusNajmabadi Nov 9, 2024

	var context = lspFileChangeWatcher.CreateContext([new ProjectSystem.WatchedDirectory(tempDirectory.Path, extensionFilters: ImmutableArray<string>.Empty)]);
	var context = lspFileChangeWatcher.CreateContext([new ProjectSystem.WatchedDirectory(tempDirectory.Path, extensionFilters: [])]);

	Public Class FileChangeWatcherTests
	Public NotInheritable Class FileChangeWatcherTests

Reduce IVsFreeThreadedFileChangeEvents2.DirectoryChangedEx2 notifications from VS shell #75815

Are you sure you want to change the base?

Reduce IVsFreeThreadedFileChangeEvents2.DirectoryChangedEx2 notifications from VS shell #75815

Conversation

ToddGrun commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Nov 8, 2024

ToddGrun commented Nov 8, 2024

CyrusNajmabadi commented Nov 8, 2024

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun commented Nov 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment