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.

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

Reduce LOH allocs when applying code fixes. #73424

Merged
merged 24 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal abstract class AbstractLinkedFileMergeConflictCommentAdditionService :
{
internal abstract string GetConflictCommentText(string header, string beforeString, string afterString);

public IEnumerable<TextChange> CreateEdits(SourceText originalSourceText, IEnumerable<UnmergedDocumentChanges> unmergedChanges)
public List<TextChange> CreateEdits(SourceText originalSourceText, List<UnmergedDocumentChanges> unmergedChanges)
Copy link
Member Author

Choose a reason for hiding this comment

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

changed a lot of the APIs to not be IEnumerable-bases (as it's impossible to tell what is lazy and waht isn't). this also means things like enumerations don't allocate.

{
var commentChanges = new List<TextChange>();

Expand All @@ -31,9 +31,9 @@ public IEnumerable<TextChange> CreateEdits(SourceText originalSourceText, IEnume
return commentChanges;
}

private static IEnumerable<IEnumerable<TextChange>> PartitionChangesForDocument(IEnumerable<TextChange> changes, SourceText originalSourceText)
private static List<List<TextChange>> PartitionChangesForDocument(IEnumerable<TextChange> changes, SourceText originalSourceText)
{
var partitionedChanges = new List<IEnumerable<TextChange>>();
var partitionedChanges = new List<List<TextChange>>();
var currentPartition = new List<TextChange>
{
changes.First()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,12 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System.Collections.Generic;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis;

internal interface IMergeConflictHandler
{
IEnumerable<TextChange> CreateEdits(SourceText originalSourceText, IEnumerable<UnmergedDocumentChanges> unmergedChanges);
List<TextChange> CreateEdits(SourceText originalSourceText, List<UnmergedDocumentChanges> unmergedChanges);
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ internal async Task<LinkedFileMergeSessionResult> MergeDiffsAsync(IMergeConflict
}

private async Task<LinkedFileMergeResult> MergeLinkedDocumentGroupAsync(
IEnumerable<DocumentId> allLinkedDocuments,
IEnumerable<DocumentId> linkedDocumentGroup,
ImmutableArray<DocumentId> allLinkedDocuments,
IGrouping<string, DocumentId> linkedDocumentGroup,
LinkedFileDiffMergingSessionInfo sessionInfo,
IMergeConflictHandler mergeConflictHandler,
CancellationToken cancellationToken)
Expand All @@ -73,7 +73,8 @@ private async Task<LinkedFileMergeResult> MergeLinkedDocumentGroupAsync(
// Automatically merge non-conflicting diffs while collecting the conflicting diffs

var textDifferencingService = oldSolution.Services.GetRequiredService<IDocumentTextDifferencingService>();
var appliedChanges = await textDifferencingService.GetTextChangesAsync(oldSolution.GetDocument(linkedDocumentGroup.First()), newSolution.GetDocument(linkedDocumentGroup.First()), cancellationToken).ConfigureAwait(false);
var appliedChanges = await textDifferencingService.GetTextChangesAsync(
oldSolution.GetDocument(linkedDocumentGroup.First()), newSolution.GetDocument(linkedDocumentGroup.First()), TextDifferenceTypes.Line, cancellationToken).ConfigureAwait(false);
var unmergedChanges = new List<UnmergedDocumentChanges>();

foreach (var documentId in linkedDocumentGroup.Skip(1))
Expand All @@ -93,10 +94,10 @@ private async Task<LinkedFileMergeResult> MergeLinkedDocumentGroupAsync(

// Add comments in source explaining diffs that could not be merged

IEnumerable<TextChange> allChanges;
IList<TextSpan> mergeConflictResolutionSpan = [];
ImmutableArray<TextChange> allChanges;
var mergeConflictResolutionSpan = new List<TextSpan>();

if (unmergedChanges.Any())
if (unmergedChanges.Count != 0)
{
mergeConflictHandler ??= oldSolution.GetDocument(linkedDocumentGroup.First()).GetLanguageService<ILinkedFileMergeConflictCommentAdditionService>();
var mergeConflictTextEdits = mergeConflictHandler.CreateEdits(originalSourceText, unmergedChanges);
Expand Down Expand Up @@ -125,12 +126,13 @@ private static async Task<ImmutableArray<TextChange>> AddDocumentMergeChangesAsy
CancellationToken cancellationToken)
{
var unmergedDocumentChanges = new List<TextChange>();
var successfullyMergedChanges = ArrayBuilder<TextChange>.GetInstance();
using var _ = ArrayBuilder<TextChange>.GetInstance(out var successfullyMergedChanges);

var cumulativeChangeIndex = 0;

var textchanges = await textDiffService.GetTextChangesAsync(oldDocument, newDocument, cancellationToken).ConfigureAwait(false);
foreach (var change in textchanges)
var textChanges = await textDiffService.GetTextChangesAsync(
oldDocument, newDocument, TextDifferenceTypes.Line, cancellationToken).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

switching to Line-based-diff is a major win here. We normally do word-based, which leads to massive allocations in hte VS side (they're the impl of this service).

foreach (var change in textChanges)
{
while (cumulativeChangeIndex < cumulativeChanges.Count && cumulativeChanges[cumulativeChangeIndex].Span.End < change.Span.Start)
{
Expand Down Expand Up @@ -196,25 +198,25 @@ private static async Task<ImmutableArray<TextChange>> AddDocumentMergeChangesAsy
groupSessionInfo.IsolatedDiffs++;
}

if (unmergedDocumentChanges.Any())
if (unmergedDocumentChanges.Count != 0)
{
unmergedChanges.Add(new UnmergedDocumentChanges(
unmergedDocumentChanges.AsEnumerable(),
unmergedDocumentChanges,
oldDocument.Project.Name,
oldDocument.Id));
}

return successfullyMergedChanges.ToImmutableAndFree();
return successfullyMergedChanges.ToImmutableAndClear();
}

private static IEnumerable<TextChange> MergeChangesWithMergeFailComments(
IEnumerable<TextChange> mergedChanges,
IEnumerable<TextChange> commentChanges,
IList<TextSpan> mergeConflictResolutionSpans,
private static ImmutableArray<TextChange> MergeChangesWithMergeFailComments(
ImmutableArray<TextChange> mergedChanges,
List<TextChange> commentChanges,
List<TextSpan> mergeConflictResolutionSpans,
LinkedFileGroupSessionInfo groupSessionInfo)
{
var mergedChangesList = NormalizeChanges(mergedChanges).ToList();
var commentChangesList = NormalizeChanges(commentChanges).ToList();
var mergedChangesList = NormalizeChanges(mergedChanges);
var commentChangesList = NormalizeChanges(commentChanges);

var combinedChanges = new List<TextChange>();
var insertedMergeConflictCommentsAtAdjustedLocation = 0;
Expand Down Expand Up @@ -270,17 +272,15 @@ private static IEnumerable<TextChange> MergeChangesWithMergeFailComments(
groupSessionInfo.InsertedMergeConflictComments = commentChanges.Count();
groupSessionInfo.InsertedMergeConflictCommentsAtAdjustedLocation = insertedMergeConflictCommentsAtAdjustedLocation;

return NormalizeChanges(combinedChanges);
return NormalizeChanges(combinedChanges).ToImmutableArray();
}

private static IEnumerable<TextChange> NormalizeChanges(IEnumerable<TextChange> changes)
private static IList<TextChange> NormalizeChanges(IList<TextChange> changes)
{
if (changes.Count() <= 1)
{
if (changes.Count <= 1)
return changes;
}

changes = changes.OrderBy(c => c.Span.Start);
var orderedChanges = changes.OrderBy(c => c.Span.Start).ToList();
var normalizedChanges = new List<TextChange>();

var currentChange = changes.First();
Expand Down Expand Up @@ -309,7 +309,7 @@ public void LogLinkedFileResult(LinkedFileGroupSessionInfo info)
=> LinkedFileGroups.Add(info);
}

internal class LinkedFileGroupSessionInfo
internal sealed class LinkedFileGroupSessionInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

sealed

seal the class right above this too?

{
public int LinkedDocuments;
public int DocumentsWithChanges;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,17 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis;

internal sealed class LinkedFileMergeResult(IEnumerable<DocumentId> documentIds, SourceText mergedSourceText, IEnumerable<TextSpan> mergeConflictResolutionSpans)
internal sealed class LinkedFileMergeResult(ImmutableArray<DocumentId> documentIds, SourceText mergedSourceText, List<TextSpan> mergeConflictResolutionSpans)
{
public IEnumerable<DocumentId> DocumentIds { get; internal set; } = documentIds;
public SourceText MergedSourceText { get; internal set; } = mergedSourceText;
public IEnumerable<TextSpan> MergeConflictResolutionSpans { get; } = mergeConflictResolutionSpans;
public bool HasMergeConflicts { get { return MergeConflictResolutionSpans.Any(); } }
public ImmutableArray<DocumentId> DocumentIds => documentIds;
public SourceText MergedSourceText => mergedSourceText;
public List<TextSpan> MergeConflictResolutionSpans => mergeConflictResolutionSpans;
public bool HasMergeConflicts => MergeConflictResolutionSpans.Count != 0;
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System.Collections.Generic;
using Microsoft.CodeAnalysis.Text;

Expand All @@ -13,19 +11,16 @@ internal sealed class LinkedFileMergeSessionResult
{
public Solution MergedSolution { get; }

private readonly Dictionary<DocumentId, IEnumerable<TextSpan>> _mergeConflictCommentSpans = [];
public Dictionary<DocumentId, IEnumerable<TextSpan>> MergeConflictCommentSpans => _mergeConflictCommentSpans;
public readonly Dictionary<DocumentId, List<TextSpan>> MergeConflictCommentSpans = [];

public LinkedFileMergeSessionResult(Solution mergedSolution, IEnumerable<LinkedFileMergeResult> fileMergeResults)
public LinkedFileMergeSessionResult(Solution mergedSolution, List<LinkedFileMergeResult> fileMergeResults)
{
this.MergedSolution = mergedSolution;

foreach (var fileMergeResult in fileMergeResults)
{
foreach (var documentId in fileMergeResult.DocumentIds)
{
_mergeConflictCommentSpans.Add(documentId, fileMergeResult.MergeConflictResolutionSpans);
}
MergeConflictCommentSpans.Add(documentId, fileMergeResult.MergeConflictResolutionSpans);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@

namespace Microsoft.CodeAnalysis;

internal sealed class UnmergedDocumentChanges(IEnumerable<TextChange> unmergedChanges, string projectName, DocumentId documentId)
Copy link
Contributor

Choose a reason for hiding this comment

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

internal

might as well clear out the #nullable in this file too

internal sealed class UnmergedDocumentChanges(List<TextChange> unmergedChanges, string projectName, DocumentId documentId)
{
public IEnumerable<TextChange> UnmergedChanges { get; } = unmergedChanges;
public List<TextChange> UnmergedChanges { get; } = unmergedChanges;
public string ProjectName { get; } = projectName;
public DocumentId DocumentId { get; } = documentId;
}
Loading