SourceTexts for Everyone! #11404
Merged
SourceTexts for Everyone! #11404
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
This change introduces a new RazorCSharpDocument.Text that returns a SourceText. The existing GeneratedCode property has been temporarily modified to return Text.ToString() to allow existing callers to work. This will be removed in later commits.
This change simply merges RazorHtmlDocument and DefaultRazorHtmlDocument into a single sealed RazorHtmlDocument class.
This change introduces a RazorHtmlDocument.Text property that is similar to RazorCSharpDocument.Text. Like the RazorCSharpDocument, the GeneratedCode property delegates to RazorHtmlDocument.Text and will be removed once callers are updated in further commits.
This change adds a Text property to IRazorGeneratedDocument that returns SourceText. The GeneratedCode property will be removed in later commits.
Updates RazorSourceGenerator to use the RazorCSharpDocument.Text property rather than GeneratedCode. This avoids some inefficiencies and removes some hidden allocations: - Passing a SourceText to CSharpSyntaxTree.ParseText(...) means that it won't create a new SourceText under-the hood to wrap the generated code. - SourceText.ContentEquals(...) can be used to compare the generated code instead of string.Equals(...). - IncrementalGeneratorInitializationContext.AddSource(...) no longer needs creates new SourceText under-the-hood to wrap the generated code. To ensure the SourceText has value equality when it flows through the incremental source generator, it's wrapped in a record struct that compares with SourceText.ContentEquals(...)
The RazorSourceGeneratorTests rely on an EventListener which can make them a challenge to debug. During debug-time, it's for the EventListener to capture spurious events, resulting in unexpected failures. This change adds a sanity check to ensure that the EventListener only captures events from the expected source. In addition, the events are no longer captures in a ConcurrentQueue, since the "queue-like" behaviors were never utilitized.
This change updates the GetCSharpSourceText() and GetHtmlSourceText() helpers to return GetCSharpDocument().Text and GetHtmlDocument().Text directly. They no longer cache objects on RazorCodeDocument.Items.
Instead of creating a new SourceText, GeneratedDocumentTextLoader.LoadTextAndVersionAsync(...) can just use RazorCSharpDocument.Text.
IRazorCodeDocument.GeneratedCode in the legacy editor shim has been changed to use RazorCSharpDocument.Text.ToString(). It caches the result to avoid constructing the string again on subsequent calls.
This change adjusts lots of code in tooling to avoid accessing RazorCSharpDocument.GeneratedCode just to call CSharpSyntaxTree.ParseText(...). RazorCodeDocument has an extension method helper for this purpose that caches the result.
This change removes the GeneratedCode property from IRazorGeneratedDocument, RazorCShaprDocument, and RazorHtmlDocument.
This keeps set up and clean up code close by.
Update compiler tests to call CodeWriter.GenerateCode().ToString() instead.
These were incorrectly defined to return ReadOnlySpan<T> and ReadOnlyMemory<T> but _should_ return Span<T> and Memory<T> to match the equivalent BCL methods.
This is a much more significant change that adds a custom TextReader to read the contents of CodeWriter's data structures. When passed to SourceText.From, it is used to either produce a StringText (by calling TextReader.ReadToEnd()) or, if the length is too large, a LargeText that wraps multiple arrays of chars rather than a giant string. The TextReader provides overrides of the methods that will ultimately be called by SourceText.From.
davidwengier
approved these changes
Jan 19, 2025
Yes -- thanks! |
jjonescz
approved these changes
Jan 20, 2025
...ared/Microsoft.AspNetCore.Razor.Test.Common/Language/IntegrationTests/IntegrationTestBase.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs
Outdated
Show resolved
Hide resolved
This was referenced Jan 23, 2025
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.
Fixes #8076
This is a change I've been planning for quite a while, and I'm hoping it will have a positive impact on performance. Essentially, this does away with that
CodeWriter.GeneratedCode,RazorCSharpDocument.GeneratedCode, andRazorHtmlDocument.GeneratedCodereturns a giant string. Instead, each provides a way to get at a canonicalSourceText. TheSourceTextreturned byCodeWriter.GetText()is produced efficiently by passing a customTextReaderintoSourceText.From(...), and the newRazorCSharpDocument.TextandRazorHtmlDocument.Textproperties just return theSourceTextthatCodeWriterproduced.In generally, I'm hoping this will reduce LOH allocations for large Razor files. However, while moving code that had previously accessed
GeneratedCodeto now use theSourceText, I found a few places to make additional performance wins:SourceText.ContentEquals(...)GeneratedCodeproperty and passing it toCSharpSyntaxTree.ParseText(...). These have all been updated and many of them were updated to just call tooling'sRazorCodeDocument.GetOrParseCSharpSyntaxTree(...), which caches the C#SytntaxTree.I focused on making the commit history high quality, so I recommend reviewing commit-by-commit if that makes sense for you.
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2623654&view=results
Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/604233