Don't refresh dynamic files under a lock #78775
Conversation
It's only for dynamic files, so make that clear.
When we were getting the notification of a dynamic file being changed, we were taking the project and workspace locks before fetching the actual content. This is really expensive now that the fetching itself might not be instant. Fundamentally this change is just switching the work to acquire the locks to get the setup info, then releasing the locks and calling the provider to get the info. I'm putting this around a batching queue so we can deduplicate lots of events at once, and also to ensure ordering so if we were to try refreshing the document more than once we don't end up in a case where we have two threads trying to put different versions of the document into the workspace and getting the ordering wrong. Fixes dotnet#78734
jasonmalinowski
force-pushed
the
dont-fetch-dynamic-files-under-locks
branch
from
1b03d1c to
efbbe24
Compare
| return; | ||
| } | ||
| return new AsyncBatchingWorkQueue<(string, string)>( | ||
| TimeSpan.FromMilliseconds(200), // 200 chosen with absolutely no evidence whatsoever |
There was a problem hiding this comment.
This constant without proof is the same constant used for regular file change notifications (and with the same comment!)
| // 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; | ||
| } |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
...ces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.BatchingDocumentCollection.cs
Show resolved
Hide resolved
|
|
||
| if (_documentPathsToDocumentIds.TryGetValue(workspaceFilePath, out var documentId)) | ||
| using (await _project._gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
When we were getting the notification of a dynamic file being changed, we were taking the project and workspace locks before fetching the actual content. This is really expensive now that the fetching itself might not be instant.
Fundamentally this change is just switching the work to acquire the locks to get the setup info, then releasing the locks and calling the provider to get the info. I'm putting this around a batching queue so we can deduplicate lots of events at once, and also to ensure ordering so if we were to try refreshing the document more than once we don't end up in a case where we have two threads trying to put different versions of the document into the workspace and getting the ordering wrong.
Fixes #78734