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

Merge touch document actions #73399

Merged
merged 2 commits into from
May 9, 2024
Merged

Merge touch document actions #73399

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d8b8a6
Merge touch document actions
CyrusNajmabadi May 9, 2024
bd4bf01
Merge remote-tracking branch 'upstream/main' into touchDocuments
CyrusNajmabadi May 9, 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
87 changes: 20 additions & 67 deletions ...es/Core/Portable/Workspace/Solution/SolutionCompilationState.TranslationAction_Actions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Diagnostics;
Expand All @@ -15,24 +16,29 @@ internal partial class SolutionCompilationState
{
private abstract partial class TranslationAction
{
internal sealed class TouchDocumentAction(
internal sealed class TouchDocumentsAction(
ProjectState oldProjectState,
ProjectState newProjectState,
DocumentState oldState,
Copy link
Member Author

Choose a reason for hiding this comment

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

passing in oldState/oldStates is redundant given that we have oldProjectState.

DocumentState newState)
: TranslationAction(oldProjectState, newProjectState)
ImmutableArray<DocumentState> newStates) : TranslationAction(oldProjectState, newProjectState)
{
private readonly DocumentState _oldState = oldState;
private readonly DocumentState _newState = newState;
private readonly ImmutableArray<DocumentState> _oldStates = newStates.SelectAsArray(s => oldProjectState.DocumentStates.GetRequiredState(s.Id));
Copy link
Contributor

Choose a reason for hiding this comment

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

_oldStates

why have this as a member? Can't each entry just be calculated inside the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be. but then i needed this here and for the sequence-equals. so basically, it was just nice to have the member.

note: if there's a problem here, we could always change this to be a OneOrMany. It's highly likely that thsi will be the 'One' case for the normal mode of a user editing a document. So we'd save on the array in that case. That said. I think this is unlikely to matter in practice since the arrays will be small, and GC'ed asap. So i'd like to keep this as-is for simplicity.

private readonly ImmutableArray<DocumentState> _newStates = newStates;

public override async Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken)
{
return oldCompilation.ReplaceSyntaxTree(
await _oldState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false),
await _newState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false));
}
var finalCompilation = oldCompilation;
for (int i = 0, n = _newStates.Length; i < n; i++)
{
cancellationToken.ThrowIfCancellationRequested();
var newState = _newStates[i];
var oldState = _oldStates[i];
finalCompilation = finalCompilation.ReplaceSyntaxTree(
await oldState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false),
await newState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false));
}

public DocumentId DocumentId => _newState.Attributes.Id;
return finalCompilation;
}

/// <summary>
/// Replacing a single tree doesn't impact the generated trees in a compilation, so we can use this against
Expand All @@ -45,71 +51,18 @@ public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generat

public override TranslationAction? TryMergeWithPrior(TranslationAction priorAction)
{
if (priorAction is TouchDocumentAction priorTouchAction &&
priorTouchAction._newState == _oldState)
if (priorAction is TouchDocumentsAction priorTouchAction &&
priorTouchAction._newStates.SequenceEqual(_oldStates))
Copy link
Contributor

Choose a reason for hiding this comment

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

priorTouchAction._newStates.SequenceEqual(_oldStates)

Is this more likely to happen than I think it is? (seems like a never would happen thing to me)

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it's very likely. this is what happens when you get a stream of edits coming in for one document (basically, the typing scenario).

we can either keep making a longer and longer chain of 'edits' where the compilation goes through N steps to get to hte end. Or we can say that the actions 'merge' and we'll jump from teh starting compilation all the way to teh end step.

So, instead of A->B->C->D, we can collapse as we're adding these and end up with just an A->D transition.

{
// As we're merging ourselves with the prior touch action, we want to keep the old project state
// that we are translating from.
return new TouchDocumentAction(priorAction.OldProjectState, this.NewProjectState, priorTouchAction._oldState, _newState);
return new TouchDocumentsAction(priorAction.OldProjectState, this.NewProjectState, _newStates);
}

return null;
}
}

internal sealed class TouchDocumentsAction : TranslationAction
{
private readonly ImmutableArray<DocumentState> _newStates;

private TouchDocumentsAction(
ProjectState oldProjectState,
ProjectState newProjectState,
ImmutableArray<DocumentState> newStates)
: base(oldProjectState, newProjectState)
{
_newStates = newStates;
}

public static TranslationAction Create(
ProjectState oldProjectState,
ProjectState newProjectState,
ImmutableArray<DocumentState> newStates)
{
// Special case when we're only updating a single document. This case can be optimized more, and
// corresponds to the common case of a single file being edited.
return newStates.Length == 1
? new TouchDocumentAction(oldProjectState, newProjectState, oldProjectState.DocumentStates.GetRequiredState(newStates[0].Id), newStates[0])
: new TouchDocumentsAction(oldProjectState, newProjectState, newStates);
}

public override async Task<Compilation> TransformCompilationAsync(Compilation oldCompilation, CancellationToken cancellationToken)
{
var finalCompilation = oldCompilation;
for (int i = 0, n = _newStates.Length; i < n; i++)
{
cancellationToken.ThrowIfCancellationRequested();
var newState = _newStates[i];
var oldState = this.OldProjectState.DocumentStates.GetRequiredState(newState.Id);
finalCompilation = finalCompilation.ReplaceSyntaxTree(
await oldState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false),
await newState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false));
}

return finalCompilation;
}

/// <inheritdoc cref="TouchDocumentAction.CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput"/>
public override bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput
=> true;

/// <inheritdoc cref="TouchDocumentAction.TransformGeneratorDriver"/>
public override GeneratorDriver TransformGeneratorDriver(GeneratorDriver generatorDriver)
=> generatorDriver;

public override TranslationAction? TryMergeWithPrior(TranslationAction priorAction)
=> null;
}

internal sealed class TouchAdditionalDocumentAction(
ProjectState oldProjectState,
ProjectState newProjectState,
Expand Down
8 changes: 4 additions & 4 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 @@ -735,7 +735,7 @@ internal SolutionCompilationState WithDocumentTexts(
}),
static (projectState, newDocumentStates) =>
{
return TranslationAction.TouchDocumentsAction.Create(
return new TranslationAction.TouchDocumentsAction(
projectState,
projectState.UpdateDocuments(newDocumentStates, contentChanged: true),
newDocumentStates);
Expand Down Expand Up @@ -816,7 +816,7 @@ public SolutionCompilationState WithDocumentSyntaxRoots(ImmutableArray<(Document
}),
static (projectState, newDocumentStates) =>
{
return TranslationAction.TouchDocumentsAction.Create(
return new TranslationAction.TouchDocumentsAction(
projectState,
projectState.UpdateDocuments(newDocumentStates, contentChanged: true),
newDocumentStates);
Expand Down Expand Up @@ -899,10 +899,10 @@ private SolutionCompilationState UpdateDocumentState(StateChange stateChange, Do
// This function shouldn't have been called if the document has not changed
Debug.Assert(stateChange.OldProjectState != stateChange.NewProjectState);

var oldDocument = stateChange.OldProjectState.DocumentStates.GetRequiredState(documentId);
var newDocument = stateChange.NewProjectState.DocumentStates.GetRequiredState(documentId);

return new TranslationAction.TouchDocumentAction(stateChange.OldProjectState, stateChange.NewProjectState, oldDocument, newDocument);
return new TranslationAction.TouchDocumentsAction(
stateChange.OldProjectState, stateChange.NewProjectState, [newDocument]);
},
forkTracker: true,
arg: documentId);
Expand Down
Loading