Make getting a SourceText from an IDE parsed tree not hit the LOH. #73494
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| private const int CharSegmentLength = 4096; | ||
|
|
||
| // 16k characters. Equivalent to 32KB in memory. comes from SourceText char buffer size and less than large object size | ||
| public const int SourceTextLengthThreshold = 32 * 1024 / sizeof(char); |
There was a problem hiding this comment.
note: we can double this safely. If you're less than 32k chars you won't be going in the LOH either (since that triggers at 70k bytes).
CyrusNajmabadi
changed the title
|
@ToddGrun ptal. |
| (CSharpParseOptions)options, | ||
| FilePath, | ||
| Encoding, | ||
| _checksumAlgorithm); |
There was a problem hiding this comment.
just wrapping. no change.
| DirectCast(options, VisualBasicParseOptions), | ||
| FilePath, | ||
| Encoding, | ||
| _checksumAlgorithm)) |
There was a problem hiding this comment.
just wrapping. no change.
| CSharpParseOptions options, | ||
| string filePath, | ||
| Encoding? encoding, | ||
| SourceHashAlgorithm checksumAlgorithm) |
...Sharp/Portable/Workspace/LanguageServices/CSharpSyntaxTreeFactoryService.ParsedSyntaxTree.cs
Outdated
Show resolved
Hide resolved
…rpSyntaxTreeFactoryService.ParsedSyntaxTree.cs
.../Portable/Workspace/LanguageServices/VisualBasicSyntaxTreeFactoryService.ParsedSyntaxTree.vb
Outdated
Show resolved
Hide resolved
…/VisualBasicSyntaxTreeFactoryService.ParsedSyntaxTree.vb
…into nodeToText
| private const int CharArrayLength = 4 * 1024; | ||
|
|
||
| // 32k characters. Equivalent to 64KB in memory bytes. Will not be put into the LOH. | ||
| public const int SourceTextLengthThreshold = 32 * 1024; |
There was a problem hiding this comment.
in double thsi value from waht it was before.
src/Workspaces/Core/Portable/Shared/Extensions/SourceTextExtensions.cs
Outdated
Show resolved
Hide resolved
| Contract.ThrowIfTrue(chunks.Any(static (c, s) => c.Length != s, CharArrayLength)); | ||
|
|
||
| using var chunkReader = new CharArrayChunkTextReader(chunks, totalLength); | ||
| var result = SourceText.From(chunkReader, totalLength, encoding, checksumAlgorithm); |
There was a problem hiding this comment.
It feels like there is an inefficiency here. This SourceText.From call takes the reader you have built up over these chunks, detects it's large, and will call LargeText.Decode, which will create chunks over the data.
I feel like we're jumping through hoops to do this at this level, when maybe it should be at the compiler level. #Resolved
There was a problem hiding this comment.
yes. but we have no efficient paths at all. We can simultaneously explore a different way of the compiler exposing things (like wrapping an ImmutableArray<ImmutableArray<char>> chunks system), but that can happen outdie of this PR.
Note: while any approach today involves multiple copies, this approach at least makes one of the copy non LOH and also pooled.
| Contract.ThrowIfNull(value); | ||
|
|
||
| var valueSpan = value.AsSpan(); | ||
| while (valueSpan.Length > 0) |
There was a problem hiding this comment.
feel free to ignore this, but I find something like the following a bit easier to read:
var index = 0;
foreach (var chunk in _chunks)
{
var copyLength = Math.Min(value.Length - index, chunk.Length);
value.CopyTo(index, chunk, 0, copyLength);
index += chunk.Length;
}
``` #Resolved
There was a problem hiding this comment.
That seems not correct since you're restarting at the first chunk each time a string is written in.
There was a problem hiding this comment.
oh, duh, multiple write calls
There was a problem hiding this comment.
ok, my current rewrite of this doesn't really look simpler than what you've got
Takes us from:
To:
A drop of about 220MB LOH allocs.