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

Reduce IVsFreeThreadedFileChangeEvents2.DirectoryChangedEx2 notifications from VS shell #75815

Merged
merged 6 commits into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Concurrent;
using System.Collections.Immutable;
using System.Text.Json;
using Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.FileWatching;
using Microsoft.CodeAnalysis.Shared.TestHooks;
Expand Down Expand Up @@ -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)]);
ToddGrun marked this conversation as resolved.
Show resolved Hide resolved
await WaitForFileWatcherAsync(testLspServer);

var watcher = GetSingleFileWatcher(dynamicCapabilitiesRpcTarget);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,23 @@ public FileChangeContext(ImmutableArray<WatchedDirectory> watchedDirectories, Ls
// If we have any watched directories, then watch those directories directly
if (watchedDirectories.Any())
{
var directoryWatches = watchedDirectories.Select(d => new FileSystemWatcher
var directoryWatches = watchedDirectories.Select(d =>
{
GlobPattern = new RelativePattern
var pattern = "**/*" + d.ExtensionFilters.Length switch
{
BaseUri = ProtocolConversions.CreateRelativePatternBaseUri(d.Path),
Pattern = d.ExtensionFilter is not null ? "**/*" + d.ExtensionFilter : "**/*"
}
0 => string.Empty,
1 => d.ExtensionFilters[0],
_ => "{" + string.Join(',', d.ExtensionFilters) + "}"
};

return new FileSystemWatcher
{
GlobPattern = new RelativePattern
{
BaseUri = ProtocolConversions.CreateRelativePatternBaseUri(d.Path),
Pattern = pattern
}
};
}).ToArray();

_directoryWatchRegistration = new LspFileWatchRegistration(lspFileChangeWatcher, directoryWatches);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ public FileChangeContext(ImmutableArray<WatchedDirectory> watchedDirectories)
var watcher = new FileSystemWatcher(watchedDirectory.Path);
watcher.IncludeSubdirectories = true;

if (watchedDirectory.ExtensionFilter != null)
watcher.Filter = '*' + watchedDirectory.ExtensionFilter;
foreach (var filter in watchedDirectory.ExtensionFilters)
watcher.Filters.Add('*' + filter);

watcher.Changed += RaiseEvent;
watcher.Created += RaiseEvent;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,7 @@ public LoadedProject(ProjectSystemProject projectSystemProject, SolutionServices
// TODO: we only should listen for add/removals here, but we can't specify such a filter now
_projectDirectory = Path.GetDirectoryName(_projectFilePath)!;

_fileChangeContext = fileWatcher.CreateContext([
new(_projectDirectory, ".cs"),
new(_projectDirectory, ".cshtml"),
new(_projectDirectory, ".razor")
]);
_fileChangeContext = fileWatcher.CreateContext([new(_projectDirectory, [".cs", ".cshtml", ".razor"])]);
_fileChangeContext.FileChanged += FileChangedContext_FileChanged;

// Start watching for file changes for the project file as well
Expand Down
47 changes: 23 additions & 24 deletions src/VisualStudio/Core/Def/ProjectSystem/FileChangeWatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ public FileChangeWatcher(
{
_fileChangeService = fileChangeService;

// 📝 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.
jasonmalinowski marked this conversation as resolved.
Show resolved Hide resolved
_taskQueue = new AsyncBatchingWorkQueue<WatcherOperation>(
TimeSpan.Zero,
TimeSpan.FromMilliseconds(500),
ProcessBatchAsync,
listenerProvider.GetListener(FeatureAttribute.Workspace),
CancellationToken.None);
Expand Down Expand Up @@ -98,10 +98,10 @@ private readonly struct WatcherOperation
private readonly Kind _kind;

/// <summary>
/// The extension filter to apply for <see cref="Kind.WatchDirectory"/>. This value may be
/// <see langword="null"/> to disable the extension filter.
/// The extension filters to apply for <see cref="Kind.WatchDirectory"/>. This value may be
/// empty to disable the extension filter.
/// </summary>
private readonly string? _filter;
private readonly ImmutableArray<string> _filters;

/// <summary>
/// The file change flags to apply for <see cref="Kind.WatchFiles"/>.
Expand Down Expand Up @@ -139,13 +139,13 @@ private readonly struct WatcherOperation
/// </summary>
private readonly OneOrMany<string> _paths;

private WatcherOperation(Kind kind, string directory, string? filter, IVsFreeThreadedFileChangeEvents2 sink, List<uint> cookies)
private WatcherOperation(Kind kind, string directory, ImmutableArray<string> filters, IVsFreeThreadedFileChangeEvents2 sink, List<uint> cookies)
{
Contract.ThrowIfFalse(kind is Kind.WatchDirectory);
_kind = kind;

_paths = new OneOrMany<string>(directory);
_filter = filter;
_filters = filters;
_sink = sink;
_cookies = cookies;

Expand All @@ -165,7 +165,7 @@ private WatcherOperation(Kind kind, OneOrMany<string> files, _VSFILECHANGEFLAGS
_tokens = tokens;

// Other watching fields are not used for this kind
_filter = null;
_filters = ImmutableArray<string>.Empty;
_cookies = null!;
}

Expand All @@ -177,7 +177,7 @@ private WatcherOperation(Kind kind, List<uint> cookies)
_cookies = cookies;

// Other watching fields are not used for this kind
_filter = null;
_filters = ImmutableArray<string>.Empty;
_fileChangeFlags = 0;
_sink = null!;
_tokens = OneOrMany<Context.RegularWatchedFile>.Empty;
Expand All @@ -192,7 +192,7 @@ private WatcherOperation(Kind kind, OneOrMany<Context.RegularWatchedFile> tokens
_tokens = tokens;

// Other watching fields are not used for this kind
_filter = null;
_filters = ImmutableArray<string>.Empty;
_fileChangeFlags = 0;
_sink = null!;
_cookies = null!;
Expand All @@ -207,8 +207,8 @@ private enum Kind
UnwatchFiles,
}

public static WatcherOperation WatchDirectory(string directory, string? filter, IVsFreeThreadedFileChangeEvents2 sink, List<uint> cookies)
=> new(Kind.WatchDirectory, directory, filter, sink, cookies);
public static WatcherOperation WatchDirectory(string directory, ImmutableArray<string> filters, IVsFreeThreadedFileChangeEvents2 sink, List<uint> cookies)
=> new(Kind.WatchDirectory, directory, filters, sink, cookies);

public static WatcherOperation WatchFile(string path, _VSFILECHANGEFLAGS fileChangeFlags, IVsFreeThreadedFileChangeEvents2 sink, Context.RegularWatchedFile token)
=> new(Kind.WatchFiles, OneOrMany.Create(path), fileChangeFlags, sink, OneOrMany.Create(token));
Expand Down Expand Up @@ -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]);
ToddGrun marked this conversation as resolved.
Show resolved Hide resolved
}

for (var i = 0; i < op._tokens.Count; i++)
{
tokensBuilder.Add(op._tokens[i]);
}
break;

case Kind.UnwatchFiles:
for (var i = 0; i < op._tokens.Count; i++)
{
tokensBuilder.Add(op._tokens[i]);
}
break;

case Kind.UnwatchDirectories:
Expand Down Expand Up @@ -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;
ToddGrun marked this conversation as resolved.
Show resolved Hide resolved
}

public async ValueTask ApplyAsync(IVsAsyncFileChangeEx2 service, CancellationToken cancellationToken)
Expand All @@ -310,8 +304,8 @@ public async ValueTask ApplyAsync(IVsAsyncFileChangeEx2 service, CancellationTok
var cookie = await service.AdviseDirChangeAsync(_paths[0], watchSubdirectories: true, _sink, cancellationToken).ConfigureAwait(false);
_cookies.Add(cookie);

if (_filter != null)
await service.FilterDirectoryChangesAsync(cookie, [_filter], cancellationToken).ConfigureAwait(false);
if (_filters != null)
Copy link
Member

Choose a reason for hiding this comment

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

Did you meant to check for non-empty, not non-null/default if this is an ImmutableArray? I imagine this will work no matter what (FilterDirectoryChangesAsync says an empty array means no filter) but it's either an unnecessary call, or an unnecessary check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an oops when converting from a nullable type to an ImmutableArray. Good catch!

await service.FilterDirectoryChangesAsync(cookie, _filters.ToArray(), cancellationToken).ConfigureAwait(false);

return;

Expand Down Expand Up @@ -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(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

watchedDirectory.Path,
watchedDirectory.ExtensionFilters,
this,
_directoryWatchCookies);

_fileChangeWatcher._taskQueue.AddWork(item);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ static ImmutableArray<WatchedDirectory> GetAdditionalWatchedDirectories()
referenceDirectories.Add(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".nuget", "packages"));
}

return referenceDirectories.SelectAsArray(static d => new WatchedDirectory(d, ".dll"));
return referenceDirectories.SelectAsArray(static d => new WatchedDirectory(d, [".dll"]));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Collections.Immutable;
using System.Linq;

namespace Microsoft.CodeAnalysis.ProjectSystem;

Expand All @@ -26,7 +27,7 @@ internal interface IFileChangeWatcher
/// </remarks>
internal sealed class WatchedDirectory
{
public WatchedDirectory(string path, string? extensionFilter)
public WatchedDirectory(string path, ImmutableArray<string> extensionFilters)
{
// We are doing string comparisons with this path, so ensure it has a trailing directory separator so we don't get confused with sibling
// paths that won't actually be covered. For example, if we're watching C:\Directory we wouldn't see changes to C:\DirectorySibling\Foo.txt.
Expand All @@ -35,32 +36,32 @@ public WatchedDirectory(string path, string? extensionFilter)
path += System.IO.Path.DirectorySeparatorChar;
}

if (extensionFilter != null && !extensionFilter.StartsWith("."))
if (extensionFilters.Any(static filter => !filter.StartsWith(".")))
{
throw new ArgumentException($"{nameof(extensionFilter)} should start with a period.", nameof(extensionFilter));
throw new ArgumentException($"{nameof(extensionFilters)} should only contain entries starting with a period.", nameof(extensionFilters));
}

Path = path;
ExtensionFilter = extensionFilter;
ExtensionFilters = extensionFilters;
}

public string Path { get; }

/// <summary>
/// If non-null, only watch the directory for changes to a specific extension. String always starts with a period.
/// </summary>
public string? ExtensionFilter { get; }
public ImmutableArray<string> ExtensionFilters { get; }

public static bool FilePathCoveredByWatchedDirectories(ImmutableArray<WatchedDirectory> watchedDirectories, string filePath, StringComparison stringComparison)
{
foreach (var watchedDirectory in watchedDirectories)
{
if (filePath.StartsWith(watchedDirectory.Path, stringComparison))
{
// If ExtensionFilter is null, then we're watching for all files in the directory so the prior check
// of the directory containment was sufficient. If it isn't null, then we have to check the extension
// If ExtensionFilters is empty, then we're watching for all files in the directory so the prior check
// of the directory containment was sufficient. If it isn't empty, then we have to check the extension
// matches.
if (watchedDirectory.ExtensionFilter == null || filePath.EndsWith(watchedDirectory.ExtensionFilter, stringComparison))
if (watchedDirectory.ExtensionFilters.Length == 0 || watchedDirectory.ExtensionFilters.Any(filter => filePath.EndsWith(filter, stringComparison)))
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,8 @@ static ImmutableArray<WatchedDirectory> GetWatchedDirectories(string? language,

return language switch
{
LanguageNames.VisualBasic => [new(rootPath, ".vb")],
LanguageNames.CSharp => [new(rootPath, ".cs"), new(rootPath, ".razor"), new(rootPath, ".cshtml")],
LanguageNames.VisualBasic => [new(rootPath, [".vb"])],
LanguageNames.CSharp => [new(rootPath, [".cs", ".razor", ".cshtml"])],
_ => []
};
}
Expand Down
Loading