Reduce allocations from newline information allocated by ChangedText.GetLinesCore #74728
Conversation
…GetLinesCore. Going to start this in draft mode and fire off a test insertion to verify this improves allocations as the typing scenario in speedometer scrolling test shows this as nearly 10% of allocations. The general idea here is that ChangedText doesn't need to keep track of line information as the SourceText that it wraps has that information. The complexity that was in ChangedText around newline splitting now needs to sit in both CompositeText and SubText as they need to understand how to expose their line collections.
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Insertion PRs to get perf numbers: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/571281 Results look promising: *** from profile *** *** from PIT *** Due to this information, I'll go ahead and elevate out of draft status. |
|
ok. i'm going to hold off on reviewing for a bit. my brain is not up to this sort of math :) docs would be helpful, as well as intuitions fro what each op is doing. also happy to talk in person to get a rundown on all of this. the savings are amazing. so def want this to move forward. |
…over all possible source texts generatable from that sequence 2) Move the two added TextLineCollection derivations to be private.
|
@RikkiGibson, @dotnet/roslyn-compiler for reviews |
…handling in two different locations, just handle it in a single location up front.
|
|
||
| var underlyingTextLine = _subText.UnderlyingText.Lines[lineNumber + _startLineNumber]; | ||
|
|
||
| var startInUnderlyingText = Math.Max(underlyingTextLine.Start, _subText.UnderlyingSpan.Start); |
There was a problem hiding this comment.
The table itself makes sense to me, but I don't understand the meaning/purpose of startInUnderlyingText. Will revisit on Monday and complete review. Thanks!
|
@dotnet/roslyn-compiler for 2nd review |
1 similar comment
|
@dotnet/roslyn-compiler for 2nd review |
| { | ||
| Debug.Assert(_subText.UnderlyingText[underlyingSpanStart - 1] == '\r' && _subText.UnderlyingText[underlyingSpanStart] == '\n'); | ||
| _startsWithinSplitCRLF = true; | ||
| } |
There was a problem hiding this comment.
Why don't we set _startsWithinSplitCRLF when we're dealing with a zero length value? Even if it's zero length I would assume it could start and end within a CRLF
There was a problem hiding this comment.
That's a good observation. I think this is a remnant of an earlier commit that needed this, but it doesn't look like that's the case any longer.
There was a problem hiding this comment.
@jaredpar -- any more concerns? Should I ping compiler team again for 2nd review?
…ome data in the empty SubText.UnderlyingSpan case.
Going to start this in draft mode and fire off a test insertion to verify this improves allocations as the typing scenario in speedometer scrolling test shows this as nearly 10% of allocations.
The general idea here is that ChangedText doesn't need to keep track of line information as the SourceText that it wraps has that information. The complexity that was in ChangedText around newline splitting now needs to sit in both CompositeText and SubText as they need to understand how to expose their line collections.
*** original allocations during the typing scenario in the scrolling speedometer test ***
