From 42acfe43adfffdbb1f9b1f348637d6e8d0c8e79c Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 17 Nov 2015 17:57:47 -0800 Subject: [PATCH] Fix aspnet/Mvc#3196 Razor Compilation Allocations This change significantly reduces the amount of string and List allocations that occur during compilation by changing the way LiteralChunks are combined. This is a low impact fix that addresses the performance issue, the design issues that caused it still exist. The problem here lies in what Razor fundamentally does - it parses HTML/C# into tokens, and then combines them back into 'chunks' a representation friendly to code generation. When presenting with a large block of static HTML, Razor parses it into individual HTML tokens, and then tries to join them in back into a single chunk for rendering. Due to details of Razor's representation of chunks/tokens, the process of combining literals is too expensive. Mainly, what's done here is to not try to combine instances of LiteralChunk. The process of merging them is too expensive and requires lots of interm List and string allocations. Instead we produce a new 'chunk' ParentLiteralChunk, which doesn't do so much up-front computing. Various pieces of the code that deal with LiteralChunk need to be updated to deal with ParentLiteralChunk also, which is the bulk of the changes here. Note that we still have the potential for LOH allocations to occur during codegen, but it's likely to occur O(1) for each large block of HTML instead of O(N) as it did in the old code. --- .../Chunks/ChunkTreeBuilder.cs | 48 ++++++++++++++----- .../Chunks/ParentLiteralChunk.cs | 21 ++++++++ .../CodeGenerators/CSharpCodeWriter.cs | 17 ++++++- .../CSharpTagHelperCodeRenderer.cs | 19 +++++--- .../Visitors/CSharpCodeVisitor.cs | 37 ++++++++++++++ .../CSharpTagHelperAttributeValueVisitor.cs | 13 +++-- .../CodeGenerators/Visitors/ChunkVisitor.cs | 5 ++ .../CodeGenerators/Visitors/CodeVisitor.cs | 3 ++ .../Chunks/Generators/ChunkTreeBuilderTest.cs | 43 +++++++++++++++-- 9 files changed, 176 insertions(+), 30 deletions(-) create mode 100644 src/Microsoft.AspNet.Razor/Chunks/ParentLiteralChunk.cs diff --git a/src/Microsoft.AspNet.Razor/Chunks/ChunkTreeBuilder.cs b/src/Microsoft.AspNet.Razor/Chunks/ChunkTreeBuilder.cs index 5c34de2f42..594e6c9184 100644 --- a/src/Microsoft.AspNet.Razor/Chunks/ChunkTreeBuilder.cs +++ b/src/Microsoft.AspNet.Razor/Chunks/ChunkTreeBuilder.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Diagnostics; using Microsoft.AspNet.Razor.Parser.SyntaxTree; namespace Microsoft.AspNet.Razor.Chunks @@ -69,22 +70,43 @@ namespace Microsoft.AspNet.Razor.Chunks public void AddLiteralChunk(string literal, SyntaxTreeNode association) { - // If the previous chunk was also a LiteralChunk, append the content of the current node to the previous one. - var literalChunk = _lastChunk as LiteralChunk; - if (literalChunk != null) + ParentLiteralChunk parentLiteralChunk; + + // We try to join literal chunks where possible, so that we have fewer 'writes' in the generated code. + // + // Possible cases here: + // - We just added a LiteralChunk and we need to add another - so merge them into ParentLiteralChunk. + // - We have a ParentLiteralChunk - merge the new chunk into it. + // - We just added something - just add the LiteralChunk like normal. + if (_lastChunk is LiteralChunk) { - // Literal chunks are always associated with Spans - var lastSpan = (Span)literalChunk.Association; - var currentSpan = (Span)association; - - var builder = new SpanBuilder(lastSpan); - foreach (var symbol in currentSpan.Symbols) + parentLiteralChunk = new ParentLiteralChunk() { - builder.Accept(symbol); - } + Start = _lastChunk.Start, + }; - literalChunk.Association = builder.Build(); - literalChunk.Text += literal; + parentLiteralChunk.Children.Add(_lastChunk); + parentLiteralChunk.Children.Add(new LiteralChunk + { + Association = association, + Start = association.Start, + Text = literal, + }); + + Debug.Assert(Current.Children[Current.Children.Count - 1] == _lastChunk); + Current.Children.RemoveAt(Current.Children.Count - 1); + Current.Children.Add(parentLiteralChunk); + _lastChunk = parentLiteralChunk; + } + else if ((parentLiteralChunk = _lastChunk as ParentLiteralChunk) != null) + { + parentLiteralChunk.Children.Add(new LiteralChunk + { + Association = association, + Start = association.Start, + Text = literal, + }); + _lastChunk = parentLiteralChunk; } else { diff --git a/src/Microsoft.AspNet.Razor/Chunks/ParentLiteralChunk.cs b/src/Microsoft.AspNet.Razor/Chunks/ParentLiteralChunk.cs new file mode 100644 index 0000000000..83f5858f97 --- /dev/null +++ b/src/Microsoft.AspNet.Razor/Chunks/ParentLiteralChunk.cs @@ -0,0 +1,21 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Text; + +namespace Microsoft.AspNet.Razor.Chunks +{ + public class ParentLiteralChunk : ParentChunk + { + public string GetText() + { + var builder = new StringBuilder(); + for (var i = 0; i < Children.Count; i++) + { + builder.Append(((LiteralChunk)Children[i]).Text); + } + + return builder.ToString(); + } + } +} diff --git a/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpCodeWriter.cs b/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpCodeWriter.cs index 78c35736a7..8eb490b44a 100644 --- a/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpCodeWriter.cs +++ b/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpCodeWriter.cs @@ -495,11 +495,24 @@ namespace Microsoft.AspNet.Razor.CodeGenerators ChunkGeneratorContext context, SyntaxTreeNode syntaxNode, bool isLiteral) + { + return WriteStartInstrumentationContext( + context, + syntaxNode.Start.AbsoluteIndex, + syntaxNode.Length, + isLiteral); + } + + public CSharpCodeWriter WriteStartInstrumentationContext( + ChunkGeneratorContext context, + int absoluteIndex, + int length, + bool isLiteral) { WriteStartMethodInvocation(context.Host.GeneratedClassContext.BeginContextMethodName); - Write(syntaxNode.Start.AbsoluteIndex.ToString(CultureInfo.InvariantCulture)); + Write(absoluteIndex.ToString(CultureInfo.InvariantCulture)); WriteParameterSeparator(); - Write(syntaxNode.Length.ToString(CultureInfo.InvariantCulture)); + Write(length.ToString(CultureInfo.InvariantCulture)); WriteParameterSeparator(); Write(isLiteral ? "true" : "false"); return WriteEndMethodInvocation(); diff --git a/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpTagHelperCodeRenderer.cs b/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpTagHelperCodeRenderer.cs index 5c32c3955d..30b74de65d 100644 --- a/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpTagHelperCodeRenderer.cs +++ b/src/Microsoft.AspNet.Razor/CodeGenerators/CSharpTagHelperCodeRenderer.cs @@ -397,7 +397,7 @@ namespace Microsoft.AspNet.Razor.CodeGenerators // statement together and the #line pragma correct to make debugging possible. using (var lineMapper = new CSharpLineMappingWriter( _writer, - attributeValueChunk.Association.Start, + attributeValueChunk.Start, _context.SourceFile)) { // Place the assignment LHS to align RHS with original attribute value's indentation. @@ -710,16 +710,21 @@ namespace Microsoft.AspNet.Razor.CodeGenerators return false; } - var literalChildChunk = parentChunk.Children[0] as LiteralChunk; - - if (literalChildChunk == null) + LiteralChunk literalChildChunk; + if ((literalChildChunk = parentChunk.Children[0] as LiteralChunk) != null) { - return false; + plainText = literalChildChunk.Text; + return true; } - plainText = literalChildChunk.Text; + ParentLiteralChunk parentLiteralChunk; + if ((parentLiteralChunk = parentChunk.Children[0] as ParentLiteralChunk) != null) + { + plainText = parentLiteralChunk.GetText(); + return true; + } - return true; + return false; } // A CSharpCodeVisitor which does not HTML encode values. Used when rendering bound string attribute values. diff --git a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpCodeVisitor.cs b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpCodeVisitor.cs index 9e6157175b..1556a9ad52 100644 --- a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpCodeVisitor.cs +++ b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpCodeVisitor.cs @@ -5,6 +5,7 @@ using System; using System.Diagnostics; using System.Globalization; using System.Linq; +using System.Text; using Microsoft.AspNet.Razor.Chunks; using Microsoft.AspNet.Razor.Parser.SyntaxTree; @@ -116,6 +117,42 @@ namespace Microsoft.AspNet.Razor.CodeGenerators.Visitors Writer.WriteEndMethodInvocation(false).WriteLine(); } + protected override void Visit(ParentLiteralChunk chunk) + { + Debug.Assert(chunk.Children.Count > 1); + + if (Context.Host.DesignTimeMode) + { + // Skip generating the chunk if we're in design time or if the chunk is empty. + return; + } + + var text = chunk.GetText(); + + if (Context.Host.EnableInstrumentation) + { + var start = chunk.Start.AbsoluteIndex; + Writer.WriteStartInstrumentationContext(Context, start, text.Length, isLiteral: true); + } + + if (Context.ExpressionRenderingMode == ExpressionRenderingMode.WriteToOutput) + { + RenderPreWriteStart(); + } + + Writer.WriteStringLiteral(text); + + if (Context.ExpressionRenderingMode == ExpressionRenderingMode.WriteToOutput) + { + Writer.WriteEndMethodInvocation(); + } + + if (Context.Host.EnableInstrumentation) + { + Writer.WriteEndInstrumentationContext(Context); + } + } + protected override void Visit(LiteralChunk chunk) { if (Context.Host.DesignTimeMode || string.IsNullOrEmpty(chunk.Text)) diff --git a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpTagHelperAttributeValueVisitor.cs b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpTagHelperAttributeValueVisitor.cs index cc2f1eee48..3798708a8b 100644 --- a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpTagHelperAttributeValueVisitor.cs +++ b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CSharpTagHelperAttributeValueVisitor.cs @@ -90,7 +90,7 @@ namespace Microsoft.AspNet.Razor.CodeGenerators.Visitors /// The to render. protected override void Visit(ExpressionChunk chunk) { - RenderCode(chunk.Code, (Span)chunk.Association); + RenderCode(chunk.Code, chunk.Start); } /// @@ -99,7 +99,12 @@ namespace Microsoft.AspNet.Razor.CodeGenerators.Visitors /// The to render. protected override void Visit(LiteralChunk chunk) { - RenderCode(chunk.Text, (Span)chunk.Association); + RenderCode(chunk.Text, chunk.Start); + } + + protected override void Visit(ParentLiteralChunk chunk) + { + RenderCode(chunk.GetText(), chunk.Start); } /// @@ -150,10 +155,10 @@ namespace Microsoft.AspNet.Razor.CodeGenerators.Visitors } // Tracks the code mapping and writes code for a leaf node in the attribute value Chunk tree. - private void RenderCode(string code, Span association) + private void RenderCode(string code, SourceLocation start) { _firstChild = false; - using (new CSharpLineMappingWriter(Writer, association.Start, code.Length)) + using (new CSharpLineMappingWriter(Writer, start, code.Length)) { Writer.Write(code); } diff --git a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/ChunkVisitor.cs b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/ChunkVisitor.cs index 1f79be0b17..1c678bbc64 100644 --- a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/ChunkVisitor.cs +++ b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/ChunkVisitor.cs @@ -53,6 +53,10 @@ namespace Microsoft.AspNet.Razor.CodeGenerators.Visitors { Visit((LiteralChunk)chunk); } + else if (chunk is ParentLiteralChunk) + { + Visit((ParentLiteralChunk)chunk); + } else if (chunk is ExpressionBlockChunk) { Visit((ExpressionBlockChunk)chunk); @@ -120,6 +124,7 @@ namespace Microsoft.AspNet.Razor.CodeGenerators.Visitors } protected abstract void Visit(LiteralChunk chunk); + protected abstract void Visit(ParentLiteralChunk chunk); protected abstract void Visit(ExpressionChunk chunk); protected abstract void Visit(StatementChunk chunk); protected abstract void Visit(TagHelperChunk chunk); diff --git a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CodeVisitor.cs b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CodeVisitor.cs index c55ada4e65..4eb07c977a 100644 --- a/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CodeVisitor.cs +++ b/src/Microsoft.AspNet.Razor/CodeGenerators/Visitors/CodeVisitor.cs @@ -26,6 +26,9 @@ namespace Microsoft.AspNet.Razor.CodeGenerators.Visitors protected override void Visit(LiteralChunk chunk) { } + protected override void Visit(ParentLiteralChunk chunk) + { + } protected override void Visit(ExpressionBlockChunk chunk) { } diff --git a/test/Microsoft.AspNet.Razor.Test/Chunks/Generators/ChunkTreeBuilderTest.cs b/test/Microsoft.AspNet.Razor.Test/Chunks/Generators/ChunkTreeBuilderTest.cs index cf3cf208ea..6acb1d9d0e 100644 --- a/test/Microsoft.AspNet.Razor.Test/Chunks/Generators/ChunkTreeBuilderTest.cs +++ b/test/Microsoft.AspNet.Razor.Test/Chunks/Generators/ChunkTreeBuilderTest.cs @@ -92,10 +92,45 @@ namespace Microsoft.AspNet.Razor.Chunks.Generators // Assert var chunk = Assert.Single(builder.Root.Children); - var literalChunk = Assert.IsType(chunk); - Assert.Equal("

", literalChunk.Text); - var span = Assert.IsType(literalChunk.Association); - Assert.Equal(previousSpan.Symbols.Concat(newSpan.Symbols), span.Symbols); + + var literalChunk = Assert.IsType(chunk); + Assert.Equal(2, literalChunk.Children.Count); + + var span = Assert.IsType(literalChunk.Children[0].Association); + Assert.Same(span, previousSpan); + + span = Assert.IsType(literalChunk.Children[1].Association); + Assert.Same(span, newSpan); + } + + [Fact] + public void AddLiteralChunk_CreatesNewChunk_IfChunkIsNotLiteral() + { + // Arrange + var spanFactory = SpanFactory.CreateCsHtml(); + var span1 = spanFactory.Markup("").Builder.Build(); + var span2 = spanFactory.Markup("

").Builder.Build(); + var span3 = spanFactory.Code("Hi!").AsExpression().Builder.Build(); + var builder = new ChunkTreeBuilder(); + + // Act + builder.AddLiteralChunk("", span1); + builder.AddLiteralChunk("

", span2); + builder.AddExpressionChunk("Hi!", span3); + + // Assert + Assert.Equal(2, builder.Root.Children.Count); + + var literalChunk = Assert.IsType(builder.Root.Children[0]); + Assert.Equal(2, literalChunk.Children.Count); + + var span = Assert.IsType(literalChunk.Children[0].Association); + Assert.Same(span, span1); + + span = Assert.IsType(literalChunk.Children[1].Association); + Assert.Same(span, span2); + + Assert.IsType(builder.Root.Children[1]); } [Fact]