Simplify the code for synchronizing the workspace with OOP.#61634
Simplify the code for synchronizing the workspace with OOP.#61634CyrusNajmabadi merged 8 commits intodotnet:mainfrom
Conversation
| { | ||
| private readonly Workspace _workspace; | ||
| private readonly TaskQueue _textChangeQueue; | ||
| private readonly AsyncQueue<IAsyncToken> _workQueue = new(); |
There was a problem hiding this comment.
prior system used queues without any ability for batching/debouncing. New system can do that.
| /// <summary> | ||
| /// Cancellation token we trigger when we enter the paused state. | ||
| /// </summary> | ||
| private CancellationToken _currentWorkToken; |
There was a problem hiding this comment.
pausing is controlled by a very simple cancellation-series.
| protected override async Task ExecuteAsync() | ||
| public void Shutdown() | ||
| { | ||
| lock (_gate) |
There was a problem hiding this comment.
no need for locks.
| private SolutionChecksumUpdater? _checksumUpdater; | ||
| #pragma warning disable IDE0044 // Add readonly modifier | ||
| private CancellationTokenSource _disposalCancellationSource = new(); | ||
| #pragma warning restore IDE0044 // Add readonly modifier |
There was a problem hiding this comment.
no clue why this wasn't readonly.
| var state = await oldDocument.State.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false); | ||
|
|
||
| await client.TryInvokeAsync<IRemoteAssetSynchronizationService>( | ||
| (service, cancellationToken) => service.SynchronizeTextAsync(oldDocument.Id, state.Text, textChanges, cancellationToken), |
There was a problem hiding this comment.
we could consider an entrypoint that actually takes all the docIds and the set of text changes all at once
| _workspace = workspace; | ||
| _textChangeQueue = new TaskQueue(Listener, TaskScheduler.Default); | ||
|
|
||
| _textChangeQueue = new AsyncBatchingWorkQueue<(Document? oldDocument, Document? newDocument)>( |
There was a problem hiding this comment.
So I see we have two queues because there are two different calls to synchronize the primary workspace and synchronize text changes. So question...
My naive implementation would be to just have one call that says 'SynchronizePrimaryWorkspace' that gives it the tree? of checksums, which the remote host can then diff against what it has and callback asking for what it is missing. However I'm guessing that could be expensive in the common case when one document is just changing over and over. Is that roughly a correct understanding?
There was a problem hiding this comment.
Talked offline. The reason for the text-syncing is to optimize teh common case of lots of edits happening to a file. We can just preemptively populate oop with teh contents of those docs just by sending the text changes over.
| public SolutionChecksumUpdater(Workspace workspace, IGlobalOptionService globalOptions, IAsynchronousOperationListenerProvider listenerProvider, CancellationToken shutdownToken) | ||
| : base(listenerProvider.GetListener(FeatureAttribute.SolutionChecksumUpdater), | ||
| workspace.Services.GetService<IGlobalOperationNotificationService>(), | ||
| TimeSpan.FromMilliseconds(globalOptions.GetOption(RemoteHostOptions.SolutionChecksumMonitorBackOffTimeSpanInMS)), shutdownToken) |
| // An expensive global operation started (like a build). Pause ourselves and cancel any outstanding work in | ||
| // progress to synchronize the solution. | ||
| // | ||
| // Note: We purposefully ignore the cancellation token produced by CreateNext. The purpose here is to | ||
| // cancel the token controlling the current work, but not have any uncanceled token for new work to use when | ||
| // it comes in. |
There was a problem hiding this comment.
Did you get a sense if this really does much? I have a internal perf bug that was hinting that we might be doing lots of sync operations when we are competing against other work. But even this means we're not preemptively doing a sync, any individual feature could still force the sync to happen anyways in response to any other operation, correct?
There was a problem hiding this comment.
it's a very good question. perhaps this pausing was done to prevent many syncs from happening as things like DLLs changed on disk underneath the IDE as you're doing a build. the benefit being that sure, featuers might request this, but they are also going at their slow cadence. at we aren't aggressively pushing here in that case.
but a part of me does feel like this is pretty suspect and potentially not at all beneficial.
No description provided.