Merge changes from a single DidChange notification #74268
Conversation
I'm currently investigating a customer profile where didChange notifications are showing very prevalently in CPU time. I've create this PR (dotnet#74267) to help with the underlying performance of creating the SourceText, however, there would still be potentially many source texts created for a single operation. This PR attempts to alleviate that situation by changing the DidChange handler to create a single source text on the input TextDocumentContentChangeEvent array. This array can be quite large, such as when the user does a replace-all or multi-line edit in VS. Roughly 50% of CPU time in the customer profile was spent in our DidChange handler (about 29 seconds of CPU time).
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
| @@ -243,5 +243,20 @@ private static bool TryGetVSCompletionListSetting(ClientCapabilities clientCapab | |||
|
|
|||
| return true; | |||
| } | |||
|
|
|||
| public static int CompareTo(this Position p1, Position p2) | |||
|
@dotnet/roslyn-ide -- ptal |
1 similar comment
|
@dotnet/roslyn-ide -- ptal |
|
@dibarbet or @CyrusNajmabadi -- ptal |
|
@dotnet/roslyn-ide -- ptal |
| // If the host sends us changes in a way such that no earlier change can affect the position of a later change, | ||
| // then we can merge the changes into a single TextChange, allowing creation of only a single new | ||
| // source text. | ||
| for (var i = 1; i < contentChanges.Length; i++) |
There was a problem hiding this comment.
can we add some tests for this (e.g. sending changes that overlap and don't overlap and verifying that the server ends up with the correct source text?
There was a problem hiding this comment.
Added some unit tests, they aren't as clean as I had hoped, but it should at least give some coverage
|
|
||
| context.UpdateTrackedDocument(request.TextDocument.Uri, text); | ||
| if (prevChange.Range.Start.CompareTo(curChange.Range.End) < 0) |
There was a problem hiding this comment.
Its early and I'm not 100% sure of this. Is it possible we need to compare against all previous changes? For example we have two disparate changes, then one afterwards that overlaps just the first one?
There was a problem hiding this comment.
Chatted in person, let me know if you still have concerns.
|
@dibarbet -- any remaining concerns? |
I'm currently investigating a customer profile where didChange notifications are showing very prevalently in CPU time. I've create this PR (#74267) to help with the underlying performance of creating the SourceText, however, there would still be potentially many source texts created for a single operation.
This PR attempts to alleviate that situation by changing the DidChange handler to create a single source text on the input TextDocumentContentChangeEvent array. This array can be quite large, such as when the user does a replace-all or multi-line edit in VS.
Roughly 50% of CPU time in the customer profile was spent in our DidChange handler (about 29 seconds of CPU time).