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

Jump to bottom

Avoid unnecessary array+linq allocs in common case #73727

Merged
merged 3 commits into from
May 27, 2024
Merged

Avoid unnecessary array+linq allocs in common case #73727

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38f9d6f
Avoid unnecessary array alloc in common case
CyrusNajmabadi May 26, 2024
6163db4
move to single caller
CyrusNajmabadi May 26, 2024
80206a4
Do all changes at once
CyrusNajmabadi May 27, 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
35 changes: 20 additions & 15 deletions src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,20 +708,13 @@ public SolutionCompilationState WithDocumentFilePath(
this.SolutionState.WithDocumentFilePath(documentId, filePath), documentId);
}

/// <inheritdoc cref="SolutionState.WithDocumentText(DocumentId, SourceText, PreservationMode)"/>
public SolutionCompilationState WithDocumentText(DocumentId documentId, SourceText text, PreservationMode mode)
=> WithDocumentTexts([(documentId, text, mode)]);

internal SolutionCompilationState WithDocumentTexts(
ImmutableArray<(DocumentId documentId, SourceText text, PreservationMode mode)> texts)
{
return WithDocumentContents(
texts, IsUnchanged,
internal SolutionCompilationState WithDocumentTexts(ImmutableArray<(DocumentId documentId, SourceText text, PreservationMode mode)> texts)
=> WithDocumentContents(
texts, SourceTextIsUnchanged,
static (documentState, text, mode) => documentState.UpdateText(text, mode));

static bool IsUnchanged(DocumentState oldDocument, SourceText text)
=> oldDocument.TryGetText(out var oldText) && text == oldText;
}
private static bool SourceTextIsUnchanged(DocumentState oldDocument, SourceText text)
=> oldDocument.TryGetText(out var oldText) && text == oldText;

private SolutionCompilationState WithDocumentContents<TContent>(
ImmutableArray<(DocumentId documentId, TContent content, PreservationMode mode)> texts,
Expand Down Expand Up @@ -1658,7 +1651,7 @@ public SolutionCompilationState WithCachedSourceGeneratorState(ProjectId project
/// </summary>
public SolutionCompilationState WithDocumentText(IEnumerable<DocumentId?> documentIds, SourceText text, PreservationMode mode)
{
var result = this;
using var _ = ArrayBuilder<(DocumentId, SourceText, PreservationMode)>.GetInstance(out var changedDocuments);

foreach (var documentId in documentIds)
{
Expand All @@ -1670,10 +1663,22 @@ public SolutionCompilationState WithDocumentText(IEnumerable<DocumentId?> docume

var documentState = this.SolutionState.GetProjectState(documentId.ProjectId)?.DocumentStates.GetState(documentId);
if (documentState != null)
result = result.WithDocumentText(documentId, text, mode);
{
// before allocating an array below (and calling into a function that does a fair amount of linq work),
// do a fast check if the text has actually changed. this shows up in allocation traces and is
// worthwhile to avoid for the common case where we're continually being asked to update the same doc to
// the same text (for example, when GetOpenDocumentInCurrentContextWithChanges) is called.
//
// The use of GetRequiredState mirrors what happens in WithDocumentTexts
if (!SourceTextIsUnchanged(documentState, text))
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!SourceTextIsUnchanged(documentState, text))

Sorry, another question.

Is there usually only a single documentId that will end up calling WithDocumentTexts? (if multiple will, it seems like they should be batched together)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. That definitely makes sense. I can tweak to that when I get home. Lmk if this is otherwise ok

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

changedDocuments.Add((documentId, text, mode));
}
}

return result;
if (changedDocuments.Count == 0)
return this;

return this.WithDocumentTexts(changedDocuments.ToImmutableAndClear());
}

internal TestAccessor GetTestAccessor()
Expand Down
Loading