Wait for solution change events before processing checksums #77059
Conversation
ghost
added
Area-IDE
untriaged
| { | ||
| // Wait for the workspace to finish synchronizing changes before continuing | ||
| var workspace = compilationState.Services.GetRequiredService<IWorkspaceAccessorService>().Workspace; | ||
| await workspace.WaitForScheduledTasksAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
this seems like this is gating this sync working on current workspace work completing (which might be large amounts of unrelated work).
There was a problem hiding this comment.
This only waits for workspace event handler callbacks, which should be executing quickly. It doesn't wait for second-level operation queues.
There was a problem hiding this comment.
my preference would be to wait for just the operations that send the text over. it's all a bit to disconnected here, meaning it's just too easy to break this. I would prefer (if we go this route) to have very tight coupling. The sync process says: "ensure pending text changes are sync'ed over first" then it does its work.
it would also be fine if the outgoing call system for features did this. So any outgoing rpc call (that specified a Solutino/Proj to sync) had that call wait on the text change sync.
|
|
||
| internal interface IWorkspaceAccessorService : IWorkspaceService | ||
| { | ||
| Workspace Workspace { get; } |
There was a problem hiding this comment.
this seems really strange to have a service to get this. it feels like we're not passing through data we should be if we need this.
There was a problem hiding this comment.
Accessing through HostWorkspaceServices was also available, but marked obsolete. This interface allows us to run a quick speedometer validation without spending too much time refactoring this.
| // During the first call to obtain compilation state checksums, also ensure the host waits for pending | ||
| // workspace change events to be processed. This minimizes the cost of synchronization since workspace | ||
| // change events are processed as diffs, while cache misses on the client side are processed by sending | ||
| // complete documents. |
There was a problem hiding this comment.
since workspace change events are processed as diffs
Only some workspace change events are diffs. Others are full syncs of data.
| // complete documents. | ||
| var newSolutionCompilationChecksums = await assetProvider.GetAssetAsync<SolutionCompilationStateChecksums>( | ||
| AssetPathKind.SolutionCompilationStateChecksums, solutionChecksum, cancellationToken).ConfigureAwait(false); | ||
| AssetPathKind.SolutionCompilationStateChecksums | AssetPathKind.SolutionSynchronization, solutionChecksum, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
instead of this, why nto have a dedicated call back to the host to simply say: finish sending text diffs over?
That seems to be a clearer and more narrowly defined codepath to accomplish what is being tried here, without hijacking unrelated codepaths. It also means we can do things like only waiting on the _textChangeQueue in SolutionChecksumUpdater to finish.
There was a problem hiding this comment.
instead of this, why nto have a dedicated call back to the host to simply say: finish sending text diffs over?
I thought about that, but it seems like it would add some unnecessary overhead for the case where synchronization was already complete. Since the purpose of this pull request is to review the speedometer data relative to another PR, I opted to consolidate these calls to minimize overhead. If we decide to move forward with this change, we can split it out to a separate call and avoid making that call in cases where there are no changes left to synchronize.
There was a problem hiding this comment.
If we decide to move forward with this change, we can split it out to a separate call and avoid making that call in cases where there are no changes left to synchronize.
thanks.
|
Did a test insertion and speedometer run and didn't get the results we were hoping for. During typing in VS, allocations under SerializableSourceText.Serialize went from 20.5 MB to 13 MB. During typing in the CA process, allocations under SerializableSourceText.Deserialize went from 72.6 MB to 31 MB. Definitely a nice improvement, but source text serialization is still happening during typing. Note that there were two broken tests during the run, I'm unsure if those are related to this change. |
@ToddGrun can you please continue with your PR. At whichever commit in it you feel is a reasonable balance of complexity vs savings. Thanks. |
3 participants
No description provided.