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

Pool CodeWriter ReadOnlyMemory<char> pages #10585

Merged
merged 3 commits into from
Jul 16, 2024
Merged

Pool CodeWriter ReadOnlyMemory<char> pages #10585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8ad94c
Pool CodeWriter ReadOnlyMemory<char> pages
ToddGrun Jul 4, 2024
eee4fe9
Fix issue causing test failures
ToddGrun Jul 4, 2024
96b3adf
1) Ensure CodeWriter disposal from other usage locations
ToddGrun Jul 15, 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
42 changes: 21 additions & 21 deletions src/Compiler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/CSharpCodeWriterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public static IEnumerable<object[]> NewLines
public void CSharpCodeWriter_TracksPosition_WithWrite()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234");
Expand All @@ -48,7 +48,7 @@ public void CSharpCodeWriter_TracksPosition_WithWrite()
public void CSharpCodeWriter_TracksPosition_WithIndent()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteLine();
Expand All @@ -65,7 +65,7 @@ public void CSharpCodeWriter_TracksPosition_WithIndent()
public void CSharpCodeWriter_TracksPosition_WithWriteLine()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteLine("1234");
Expand All @@ -83,7 +83,7 @@ public void CSharpCodeWriter_TracksPosition_WithWriteLine()
public void CSharpCodeWriter_TracksPosition_WithWriteLine_WithNewLineInContent(string newLine)
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteLine("1234" + newLine + "12");
Expand All @@ -104,7 +104,7 @@ public void CSharpCodeWriter_TracksPosition_WithWriteLine_WithNewLineInContent(s
public void CSharpCodeWriter_TracksPosition_WithWrite_WithNewlineInContent(string newLine)
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234" + newLine + "123" + newLine + "12");
Expand All @@ -124,7 +124,7 @@ public void CSharpCodeWriter_TracksPosition_WithWrite_WithNewlineInContent(strin
public void CSharpCodeWriter_TracksPosition_WithWrite_WithNewlineInContent_RepeatedN()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234\n\n123");
Expand All @@ -144,7 +144,7 @@ public void CSharpCodeWriter_TracksPosition_WithWrite_WithNewlineInContent_Repea
public void CSharpCodeWriter_TracksPosition_WithWrite_WithMixedNewlineInContent()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234\r123\r\n12\n1");
Expand All @@ -164,7 +164,7 @@ public void CSharpCodeWriter_TracksPosition_WithWrite_WithMixedNewlineInContent(
public void CSharpCodeWriter_TracksPosition_WithNewline_SplitAcrossWrites()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234\r");
Expand All @@ -185,7 +185,7 @@ public void CSharpCodeWriter_TracksPosition_WithNewline_SplitAcrossWrites()
public void CSharpCodeWriter_TracksPosition_WithTwoNewline_SplitAcrossWrites_R()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234\r");
Expand All @@ -206,7 +206,7 @@ public void CSharpCodeWriter_TracksPosition_WithTwoNewline_SplitAcrossWrites_R()
public void CSharpCodeWriter_TracksPosition_WithTwoNewline_SplitAcrossWrites_N()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234\n");
Expand All @@ -227,7 +227,7 @@ public void CSharpCodeWriter_TracksPosition_WithTwoNewline_SplitAcrossWrites_N()
public void CSharpCodeWriter_TracksPosition_WithTwoNewline_SplitAcrossWrites_Reversed()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("1234\n");
Expand All @@ -248,7 +248,7 @@ public void CSharpCodeWriter_TracksPosition_WithTwoNewline_SplitAcrossWrites_Rev
public void CSharpCodeWriter_TracksPosition_WithNewline_SplitAcrossWrites_AtBeginning()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("\r");
Expand All @@ -269,7 +269,7 @@ public void CSharpCodeWriter_TracksPosition_WithNewline_SplitAcrossWrites_AtBegi
public void CSharpCodeWriter_LinesBreaksOutsideOfContentAreNotCounted()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.Write("\r\nHello\r\nWorld\r\n", startIndex: 2, count: 12);
Expand All @@ -287,7 +287,7 @@ public void WriteLineNumberDirective_UsesFilePath_FromSourceLocation()
var filePath = "some-path";
var mappingLocation = new SourceSpan(filePath, 10, 4, 3, 9);

var writer = new CodeWriter();
using var writer = new CodeWriter();
var expected = $"#line 5 \"{filePath}\"" + writer.NewLine;

// Act
Expand All @@ -302,7 +302,7 @@ public void WriteLineNumberDirective_UsesFilePath_FromSourceLocation()
public void WriteField_WritesFieldDeclaration()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteField(Array.Empty<string>(), new[] { "private" }, "global::System.String", "_myString");
Expand All @@ -319,7 +319,7 @@ public void WriteField_WritesFieldDeclaration()
public void WriteField_WithModifiers_WritesFieldDeclaration()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteField(Array.Empty<string>(), new[] { "private", "readonly", "static" }, "global::System.String", "_myString");
Expand All @@ -336,7 +336,7 @@ public void WriteField_WithModifiers_WritesFieldDeclaration()
public void WriteField_WithModifiersAndSupressions_WritesFieldDeclaration()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteField(
Expand All @@ -362,7 +362,7 @@ public void WriteField_WithModifiersAndSupressions_WritesFieldDeclaration()
public void WriteAutoPropertyDeclaration_WritesPropertyDeclaration()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteAutoPropertyDeclaration(new[] { "public" }, "global::System.String", "MyString");
Expand All @@ -379,7 +379,7 @@ public void WriteAutoPropertyDeclaration_WritesPropertyDeclaration()
public void WriteAutoPropertyDeclaration_WithModifiers_WritesPropertyDeclaration()
{
// Arrange
var writer = new CodeWriter();
using var writer = new CodeWriter();

// Act
writer.WriteAutoPropertyDeclaration(new[] { "public", "static" }, "global::System.String", "MyString");
Expand All @@ -402,7 +402,7 @@ public void CSharpCodeWriter_RespectTabSetting()
o.IndentSize = 4;
});

var writer = new CodeWriter(Environment.NewLine, options);
using var writer = new CodeWriter(Environment.NewLine, options);

// Act
writer.BuildClassDeclaration(Array.Empty<string>(), "C", "", Array.Empty<string>(), Array.Empty<TypeParameter>(), context: null);
Expand All @@ -428,7 +428,7 @@ public void CSharpCodeWriter_RespectSpaceSetting()
o.IndentSize = 4;
});

var writer = new CodeWriter(Environment.NewLine, options);
using var writer = new CodeWriter(Environment.NewLine, options);

// Act
writer.BuildClassDeclaration(Array.Empty<string>(), "C", "", Array.Empty<string>(), Array.Empty<TypeParameter>(), context: null);
Expand Down
4 changes: 2 additions & 2 deletions ...piler/Microsoft.AspNetCore.Razor.Language/test/CodeGeneration/DesignTimeNodeWriterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ Render Children
public void LinePragma_Is_Adjusted_On_Windows(string fileName, string expectedFileName)
{
var writer = new DesignTimeNodeWriter();
var context = TestCodeRenderingContext.CreateDesignTime();
using var context = TestCodeRenderingContext.CreateDesignTime();

Assert.True(context.Options.RemapLinePragmaPathsOnWindows);

Expand Down Expand Up @@ -553,7 +553,7 @@ public void LinePragma_Is_Adjusted_On_Windows(string fileName, string expectedFi
public void LinePragma_Enhanced_Is_Adjusted_On_Windows(string fileName, string expectedFileName)
{
var writer = new RuntimeNodeWriter();
var context = TestCodeRenderingContext.CreateDesignTime(source: RazorSourceDocument.Create("", fileName));
using var context = TestCodeRenderingContext.CreateDesignTime(source: RazorSourceDocument.Create("", fileName));

Assert.True(context.Options.RemapLinePragmaPathsOnWindows);
Assert.True(context.Options.UseEnhancedLinePragma);
Expand Down
33 changes: 27 additions & 6 deletions src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Did you go through all CodeWriter type references to make sure we are disposing CodeWriter instances now? It looks like RazorHtmlWriter is owning a CodeWriter but not disposing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just went through and in addition to the RazorHtmlWriter usage, there were a bunch of test file usages that I changed too. Thanks!

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
Expand All @@ -10,13 +11,13 @@

namespace Microsoft.AspNetCore.Razor.Language.CodeGeneration;

public sealed partial class CodeWriter
public sealed partial class CodeWriter : IDisposable
{
// This is the size of each "page", which are arrays of ReadOnlyMemory<char>.
// This number was chosen arbitrarily as a "best guess". If changed, care should be
// taken to ensure that pages are not allocated on the LOH. ReadOnlyMemory<char>
// takes up 16 bytes, so a page size of 1000 is 16k.
private const int PageSize = 1000;
private const int MinimumPageSize = 1000;

// Rather than using a StringBuilder, we maintain a linked list of pages, which are arrays
// of "chunks of text", represented by ReadOnlyMemory<char>. This avoids copying strings
Expand Down Expand Up @@ -65,17 +66,27 @@ private void AddTextChunk(ReadOnlyMemory<char> value)
}

// If we're at the start of a page, we need to add the page first.
var lastPage = _pageOffset == 0
? _pages.AddLast(new ReadOnlyMemory<char>[PageSize]).Value
: _pages.Last!.Value;
ReadOnlyMemory<char>[] lastPage;

if (_pageOffset == 0)
{
lastPage = ArrayPool<ReadOnlyMemory<char>>.Shared.Rent(MinimumPageSize);
_pages.AddLast(lastPage);
}
else
{
lastPage = _pages.Last!.Value;
}

// Add our chunk of text (the ReadOnlyMemory<char>) and increment the offset.
lastPage[_pageOffset] = value;
_pageOffset++;

// We've reached the end of a page, so we reset the offset to 0.
// This will cause a new page to be added next time.
if (_pageOffset == PageSize)
// _pageOffset is checked against the lastPage.Length as the Rent call that
// return that array may return an array longer that MinimumPageSize.
if (_pageOffset == lastPage.Length)
alexgav marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Good change, though it's a bit subtle. Could you add a comment or two describing this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added, hopefully more obvious now.

{
_pageOffset = 0;
}
Expand Down Expand Up @@ -370,4 +381,14 @@ public string GenerateCode()
return result;
}
}

public void Dispose()
{
foreach (var page in _pages)
{
ArrayPool<ReadOnlyMemory<char>>.Shared.Return(page, clearArray: true);
}

_pages.Clear();
}
}
17 changes: 4 additions & 13 deletions ...ft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/DefaultCodeRenderingContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,11 @@ internal class DefaultCodeRenderingContext : CodeRenderingContext
private readonly PooledObject<ImmutableArray<SourceMapping>.Builder> _sourceMappingsBuilder;

public DefaultCodeRenderingContext(
CodeWriter codeWriter,
IntermediateNodeWriter nodeWriter,
RazorCodeDocument codeDocument,
DocumentIntermediateNode documentNode,
RazorCodeGenerationOptions options)
{
if (codeWriter == null)
{
throw new ArgumentNullException(nameof(codeWriter));
}

if (nodeWriter == null)
{
throw new ArgumentNullException(nameof(nodeWriter));
Expand All @@ -52,7 +46,6 @@ public DefaultCodeRenderingContext(
throw new ArgumentNullException(nameof(options));
}

CodeWriter = codeWriter;
_codeDocument = codeDocument;
_documentNode = documentNode;
Options = options;
Expand All @@ -69,12 +62,9 @@ public DefaultCodeRenderingContext(
Diagnostics.Add(diagnostics[i]);
}

var newLineString = codeDocument.Items[NewLineString];
if (newLineString != null)
{
// Set new line character to a specific string regardless of platform, for testing purposes.
codeWriter.NewLine = (string)newLineString;
}
// Set new line character to a specific string regardless of platform, for testing purposes.
var newLineString = codeDocument.Items[NewLineString] as string ?? Environment.NewLine;
CodeWriter = new CodeWriter(newLineString, options);

Items[NewLineString] = codeDocument.Items[NewLineString];
Items[SuppressUniqueIds] = codeDocument.Items[SuppressUniqueIds] ?? options.SuppressUniqueIds;
Expand Down Expand Up @@ -220,6 +210,7 @@ public override void AddLinePragma(LinePragma linePragma)
public override void Dispose()
{
_sourceMappingsBuilder.Dispose();
CodeWriter.Dispose();
}

private struct ScopeInternal
Expand Down
1 change: 0 additions & 1 deletion ...icrosoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/DefaultDocumentWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ public override RazorCSharpDocument WriteDocument(RazorCodeDocument codeDocument
}

using var context = new DefaultCodeRenderingContext(
new CodeWriter(Environment.NewLine, _options),
_codeTarget.CreateNodeWriter(),
codeDocument,
documentNode,
Expand Down
1 change: 1 addition & 0 deletions src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorHtmlWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,5 +276,6 @@ private void WriteNode<TNode>(TNode node, bool isHtml, Action<TNode> handler) wh
public void Dispose()
{
_sourceMappingsBuilder.Dispose();
Builder.Dispose();
}
}
6 changes: 2 additions & 4 deletions ...icrosoft.AspNetCore.Razor.Test.Common/Language/CodeGeneration/TestCodeRenderingContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ public static CodeRenderingContext CreateDesignTime(
RazorSourceDocument source = null,
IntermediateNodeWriter nodeWriter = null)
{
var codeWriter = new CodeWriter();
var documentNode = new DocumentIntermediateNode();
var options = RazorCodeGenerationOptions.CreateDesignTimeDefault();

Expand All @@ -40,7 +39,7 @@ public static CodeRenderingContext CreateDesignTime(
nodeWriter = new DesignTimeNodeWriter();
}

var context = new DefaultCodeRenderingContext(codeWriter, nodeWriter, codeDocument, documentNode, options);
var context = new DefaultCodeRenderingContext(nodeWriter, codeDocument, documentNode, options);
context.Visitor = new RenderChildrenVisitor(context);

return context;
Expand All @@ -52,7 +51,6 @@ public static CodeRenderingContext CreateRuntime(
RazorSourceDocument source = null,
IntermediateNodeWriter nodeWriter = null)
{
var codeWriter = new CodeWriter();
var documentNode = new DocumentIntermediateNode();
var options = RazorCodeGenerationOptions.CreateDefault();

Expand All @@ -77,7 +75,7 @@ public static CodeRenderingContext CreateRuntime(
nodeWriter = new RuntimeNodeWriter();
}

var context = new DefaultCodeRenderingContext(codeWriter, nodeWriter, codeDocument, documentNode, options);
var context = new DefaultCodeRenderingContext(nodeWriter, codeDocument, documentNode, options);
context.Visitor = new RenderChildrenVisitor(context);

return context;
Expand Down