Skip to content

Don't refresh dynamic files under a lock #78775

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

Merged
merged 2 commits into from
May 31, 2025
Merged

Don't refresh dynamic files under a lock #78775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c81a4e8
Rename field to make it clearer what it's used for
jasonmalinowski May 30, 2025
efbbe24
Don't refresh dynamic files under a lock
jasonmalinowski May 30, 2025
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
88 changes: 56 additions & 32 deletions .../Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.BatchingDocumentCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.Threading;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Workspaces.ProjectSystem;
Expand Down Expand Up @@ -69,6 +70,18 @@ private sealed class BatchingDocumentCollection
private readonly Func<Solution, DocumentId, TextLoader, Solution> _documentTextLoaderChangedAction;
private readonly WorkspaceChangeKind _documentChangedWorkspaceKind;

/// <summary>
/// An <see cref="AsyncBatchingWorkQueue"/> for processing updates to dynamic files. This is lazily created the first time we see
/// a change to process, since dynamic files are only used in certain Razor scenarios and most projects won't ever have one.
/// </summary>
/// <remarks>
/// This is used for two reasons: first, if we have a flurry of events we want to deduplicate them. But it also ensures ordering -- if we were to get a change to
/// a dynamic file while we're already processing another change, we want to ensure that the first change is processed before the second one. Otherwise we might
/// end up with the updates being applied out of order (since we're not always holding a lock while calling to the dynamic file info provider) and we might end up with
/// an old version stuck in the workspace.
/// </remarks>
private AsyncBatchingWorkQueue<(string projectSystemFilePath, string workspaceFilePath)>? _dynamicFilesToRefresh;

public BatchingDocumentCollection(ProjectSystemProject project,
Func<Solution, DocumentId, bool> documentAlreadyInWorkspace,
Action<Workspace, DocumentInfo> documentAddAction,
Expand Down Expand Up @@ -213,7 +226,7 @@ public void AddDynamicFile_NoLock(IDynamicFileInfoProvider fileInfoProvider, Dyn

_documentIdToDynamicFileInfoProvider.Add(documentId, fileInfoProvider);

if (_project._eventSubscriptionTracker.Add(fileInfoProvider))
if (_project._dynamicFileInfoProvidersSubscribedTo.Add(fileInfoProvider))
{
// subscribe to the event when we use this provider the first time
fileInfoProvider.Updated += _project.OnDynamicFileInfoUpdated;
Expand Down Expand Up @@ -439,55 +452,66 @@ await _project._projectSystemProjectFactory.ApplyBatchChangeToWorkspaceAsync((so
/// <summary>
/// Process file content changes
/// </summary>
/// <param name="projectSystemFilePath">filepath given from project system</param>
/// <param name="workspaceFilePath">filepath used in workspace. it might be different than projectSystemFilePath</param>
/// <param name="projectSystemFilePath">File path given from project system for the .cshtml file</param>
/// <param name="workspaceFilePath">File path for the equivalent .cs document used in workspace. it might be different than projectSystemFilePath.</param>
public void ProcessDynamicFileChange(string projectSystemFilePath, string workspaceFilePath)
{
using (_project._gate.DisposableWait())
InterlockedOperations.Initialize(ref _dynamicFilesToRefresh, () =>
{
// If our project has already been removed, this is a stale notification, and we can disregard.
if (_project.HasBeenRemoved)
{
return;
}
return new AsyncBatchingWorkQueue<(string, string)>(
TimeSpan.FromMilliseconds(200), // 200 chosen with absolutely no evidence whatsoever
Copy link
Member Author

Choose a reason for hiding this comment

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

This constant without proof is the same constant used for regular file change notifications (and with the same comment!)

ProcessDynamicFileChangesAsync,
EqualityComparer<(string, string)>.Default, // uses ordinal string comparison which is what we want
_project._projectSystemProjectFactory.WorkspaceListener,
_project._asynchronousFileChangeProcessingCancellationTokenSource.Token);
});

_dynamicFilesToRefresh.AddWork((projectSystemFilePath, workspaceFilePath));
}

private async ValueTask ProcessDynamicFileChangesAsync(ImmutableSegmentedList<(string projectSystemFilePath, string workspaceFilePath)> batch, CancellationToken cancellationToken)
{
foreach (var (projectSystemPath, workspaceFilePath) in batch)
{
DocumentId? documentId;
IDynamicFileInfoProvider? fileInfoProvider;

if (_documentPathsToDocumentIds.TryGetValue(workspaceFilePath, out var documentId))
using (await _project._gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
Copy link
Contributor

Choose a reason for hiding this comment

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

DisposableWaitAsync

Instead of waiting on this gate inside the loop, could we wait on it once and do a loop inside that to collect the items which need to be processed? A subsequent loop over the items to process could then do the GetDynamicFileInfoAsync/ApplyChangeToWorkspaceAsync calls

Copy link
Member Author

Choose a reason for hiding this comment

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

Waiting on #78775 (comment) -- because if we can do this in parallel generally then we'll restructure this loop entirely and do something like what you're suggesting.

If we really won't benefit from parallel processing...not sure it'll matter much.

{
// We create file watching prior to pushing the file to the workspace in batching, so it's
// possible we might see a file change notification early. In this case, toss it out. Since
// all adds/removals of documents for this project happen under our lock, it's safe to do this
// check without taking the main workspace lock. We don't have to check for documents removed in
// the batch, since those have already been removed out of _documentPathsToDocumentIds.
if (_documentsAddedInBatch.Any(d => d.Id == documentId))
{
// If our project has already been removed, this is a stale notification, and we can disregard.
if (_project.HasBeenRemoved)
return;
}
Comment on lines -456 to -464
Copy link
Member Author

Choose a reason for hiding this comment

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

The old code here was covering this explicit case for something we could bail on, and asserting the document must exist otherwise. It seemed simpler just to call TryGetValue and delete the comment. Although I believe the old code was correct, it's too much work to justify it being right when we can simplify instead.


Contract.ThrowIfFalse(_documentIdToDynamicFileInfoProvider.TryGetValue(documentId, out var fileInfoProvider));
if (!_documentPathsToDocumentIds.TryGetValue(workspaceFilePath, out documentId))
return;

_project._projectSystemProjectFactory.ApplyChangeToWorkspace(w =>
if (!_documentIdToDynamicFileInfoProvider.TryGetValue(documentId, out fileInfoProvider))
return;
}

// Now that we've got all our basic data, let's fetch the new document outside the lock, since this could be expensive.
var fileInfo = await fileInfoProvider.GetDynamicFileInfoAsync(
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider parallelizing the fetch of the dynamic files in the batch? And then maybe we could also apply all the documents in the batch at once, avoiding needing to take the workspace lock for each 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.

Good question, that might give another speed up. @alexgav any thoughts on what might be beneficial here terms of whether there won't be some other queueing, and how much we might want to fan this out before we spam you all like crazy?

I won't block on changing this since this was already not doing parallelizing right now (so this isn't making it worse).

_project.Id, _project._filePath, projectSystemPath, CancellationToken.None).ConfigureAwait(false);
Contract.ThrowIfNull(fileInfo, "We previously received a dynamic file for this path, and we're responding to a change, so we expect to get a new one.");

await _project._projectSystemProjectFactory.ApplyChangeToWorkspaceAsync(w =>
{
if (w.IsDocumentOpen(documentId))
{
return;
}

// we do not expect JTF to be used around this code path. and contract of fileInfoProvider is it being real free-threaded
// meaning it can't use JTF to go back to UI thread.
// so, it is okay for us to call regular ".Result" on a task here.
var fileInfo = fileInfoProvider.GetDynamicFileInfoAsync(
_project.Id, _project._filePath, projectSystemFilePath, CancellationToken.None).WaitAndGetResult_CanCallOnBackground(CancellationToken.None);

Contract.ThrowIfNull(fileInfo, "We previously received a dynamic file for this path, and we're responding to a change, so we expect to get a new one.");
// Right now we're only supporting dynamic files as actual source files, so it's OK to call GetDocument here.
// If the document is longer present, that could mean we unloaded the project, or the dynamic file was removed while we had released the lock.
var documentToReload = w.CurrentSolution.GetDocument(documentId);

// Right now we're only supporting dynamic files as actual source files, so it's OK to call GetDocument here
var attributes = w.CurrentSolution.GetRequiredDocument(documentId).State.Attributes;
if (documentToReload is null)
return;

var documentInfo = new DocumentInfo(attributes, fileInfo.TextLoader, fileInfo.DocumentServiceProvider);
var documentInfo = new DocumentInfo(documentToReload.State.Attributes, fileInfo.TextLoader, fileInfo.DocumentServiceProvider);

w.OnDocumentReloaded(documentInfo);
});
}
}, cancellationToken).ConfigureAwait(false);
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ internal sealed partial class ProjectSystemProject
private readonly IFileChangeContext _documentFileChangeContext;

/// <summary>
/// track whether we have been subscribed to <see cref="IDynamicFileInfoProvider.Updated"/> event
/// The set of dynamic file info providers we have already subscribed to.
/// </summary>
private readonly HashSet<IDynamicFileInfoProvider> _eventSubscriptionTracker = [];
private readonly HashSet<IDynamicFileInfoProvider> _dynamicFileInfoProvidersSubscribedTo = [];

/// <summary>
/// Map of the original dynamic file path to the <see cref="DynamicFileInfo.FilePath"/> that was associated with it.
Expand Down Expand Up @@ -1417,12 +1417,12 @@ public void RemoveFromWorkspace()
_asynchronousFileChangeProcessingCancellationTokenSource.Cancel();

// clear tracking to external components
foreach (var provider in _eventSubscriptionTracker)
foreach (var provider in _dynamicFileInfoProvidersSubscribedTo)
{
provider.Updated -= OnDynamicFileInfoUpdated;
}

_eventSubscriptionTracker.Clear();
_dynamicFileInfoProvidersSubscribedTo.Clear();
}

_documentFileChangeContext.Dispose();
Expand Down
Loading