Use ITextDifferencingService.DiffSnapshotSpans instead of allocating full buffer text during codeaction previews #74114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…odeaction previews This is enabled by changing ChangedSourceText to use the base class's ITextSnapshot based ctor instead of the ITextImage one. The base class then keeps a mapping of ITextImage -> ITextSnapshot, that can be used during the preview to map from a SourceText to an ITextSnapshot. Having the ITextSnapshot available allows the code to use the DiffService call that takes in SnapshotSpans instead of needing to allocate a string for the whole sourcetext. The obvious concern here would be whether switching ChangedSourceText to have the ITextImage -> ITextSnapshot weakly kept is going to root the ITextSnapshot for longer than it would before. It appears that isn't too much of a worry though, as the SnapshotSourceText ctor that takes in the snapshot only uses it for the CWT mapping, and doesn't store it otherwise.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -35,14 +35,14 @@ private class SnapshotSourceText : SourceText | |||
| private readonly Encoding? _encoding; | |||
| private readonly TextBufferContainer? _container; | |||
|
|
|||
| private SnapshotSourceText(ITextBufferCloneService? textBufferCloneService, ITextSnapshot editorSnapshot, SourceHashAlgorithm checksumAlgorithm, TextBufferContainer container) | |||
| private SnapshotSourceText(ITextBufferCloneService? textBufferCloneService, ITextSnapshot editorSnapshot, Encoding? encoding, SourceHashAlgorithm checksumAlgorithm, TextBufferContainer container) | |||
There was a problem hiding this comment.
i'm missing the callsite that passed in a non-null value here. or was this just to make it explicit that all callsites are trying to pass that in?
There was a problem hiding this comment.
The ChangedSourceText.ctor was previously calling the ITextImage version of this ctor, and passing in an encoding. I wanted to match that behavior.
| var differenceOptions = new StringDifferenceOptions() | ||
| { | ||
| DifferenceType = StringDifferenceTypes.Line, | ||
| }; |
There was a problem hiding this comment.
nit. make static readonly member of this type.
| ? diffService.DiffSnapshotSpans(oldTextSnapshot!.GetFullSpan(), newTextSnapshot!.GetFullSpan(), differenceOptions) | ||
| : diffService.DiffStrings(oldText.ToString(), newText.ToString(), differenceOptions); | ||
|
|
||
| return diffResult; |
There was a problem hiding this comment.
nit: make extension on IDiffService since you dupicated this in two places
CyrusNajmabadi
approved these changes
Jun 26, 2024
This was referenced Jul 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is enabled by changing ChangedSourceText to use the base class's ITextSnapshot based ctor instead of the ITextImage one. The base class then keeps a mapping of ITextImage -> ITextSnapshot, that can be used during the preview to map from a SourceText to an ITextSnapshot. Having the ITextSnapshot available allows the code to use the DiffService call that takes in SnapshotSpans instead of needing to allocate a string for the whole sourcetext.
The obvious concern here would be whether switching ChangedSourceText to have the ITextImage -> ITextSnapshot weakly kept is going to root the ITextSnapshot for longer than it would before. It appears that isn't too much of a worry though, as the SnapshotSourceText ctor that takes in the snapshot only uses it for the CWT mapping, and doesn't store it otherwise.
This shows as about 1.5% of allocations in the profile I took that brough up lightbulb/preview multiple times.
*** Previous allocations ***
