Skip to content
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

Avoid LOH allocation in text synchronization #73425

Merged
merged 2 commits into from
May 13, 2024
Merged

Avoid LOH allocation in text synchronization #73425

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f754a6c
Avoid LOH allocation in text synchronizatin
CyrusNajmabadi May 10, 2024
ddad624
Add back length check
CyrusNajmabadi May 10, 2024
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
43 changes: 17 additions & 26 deletions src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,45 +205,36 @@ private async ValueTask SynchronizeTextChangesAsync(

async ValueTask SynchronizeTextChangesAsync(Document oldDocument, Document newDocument, CancellationToken cancellationToken)
{
// this pushes text changes to the remote side if it can.
// this is purely perf optimization. whether this pushing text change
// worked or not doesn't affect feature's functionality.
// this pushes text changes to the remote side if it can. this is purely perf optimization. whether this
// pushing text change worked or not doesn't affect feature's functionality.
//
// this basically see whether it can cheaply find out text changes
// between 2 snapshots, if it can, it will send out that text changes to
// remote side.
// this basically see whether it can cheaply find out text changes between 2 snapshots, if it can, it will
// send out that text changes to remote side.
//
// the remote side, once got the text change, will again see whether
// it can use that text change information without any high cost and
// create new snapshot from it.
// the remote side, once got the text change, will again see whether it can use that text change information
// without any high cost and create new snapshot from it.
//
// otherwise, it will do the normal behavior of getting full text from
// VS side. this optimization saves times we need to do full text
// synchronization for typing scenario.
// otherwise, it will do the normal behavior of getting full text from VS side. this optimization saves
// times we need to do full text synchronization for typing scenario.

if ((oldDocument.TryGetText(out var oldText) == false) ||
(newDocument.TryGetText(out var newText) == false))
if (!oldDocument.TryGetText(out var oldText) ||
!newDocument.TryGetText(out var newText))
{
// we only support case where text already exist
return;
}

// get text changes
var textChanges = newText.GetTextChanges(oldText).AsImmutable();
if (textChanges.Length == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

if (textChanges.Length == 0)

Does this still short-circuit in the no changes case?

{
// no changes
// Avoid allocating text before seeing if we can bail out.
var changeRanges = newText.GetChangeRanges(oldText).AsImmutable();
if (changeRanges.Length == 0)
return;
}

// whole document case
if (textChanges.Length == 1 && textChanges[0].Span.Length == oldText.Length)
{
// no benefit here. pulling from remote host is more efficient
// no benefit here. pulling from remote host is more efficient
if (changeRanges is [{ Span.Length: var singleChangeLength }] && singleChangeLength == oldText.Length)
return;
}

// only cancelled when remote host gets shutdown
var textChanges = newText.GetTextChanges(oldText).AsImmutable();

var client = await RemoteHostClient.TryGetClientAsync(_workspace, cancellationToken).ConfigureAwait(false);
if (client == null)
return;
Expand Down
Loading