From 1ce9180a3eace0134cc63f168dff57efc93cf294 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Fri, 15 Jan 2016 12:27:21 -0800 Subject: [PATCH] Prevent unneeded allocations of `Block` children enumerators. #635 --- .../Parser/ConditionalAttributeCollapser.cs | 16 ++++- .../Parser/ParserContext.cs | 2 +- .../Parser/ParserVisitor.cs | 6 +- .../Parser/SyntaxTree/Block.cs | 16 +++-- .../Parser/SyntaxTree/BlockBuilder.cs | 2 +- .../TagHelpers/TagHelperParseTreeRewriter.cs | 19 ++++-- .../Framework/BlockTypes.cs | 65 ++++++------------- .../Framework/ParserTestBase.cs | 2 +- 8 files changed, 66 insertions(+), 62 deletions(-) diff --git a/src/Microsoft.AspNet.Razor/Parser/ConditionalAttributeCollapser.cs b/src/Microsoft.AspNet.Razor/Parser/ConditionalAttributeCollapser.cs index 73be90198c..e13349de26 100644 --- a/src/Microsoft.AspNet.Razor/Parser/ConditionalAttributeCollapser.cs +++ b/src/Microsoft.AspNet.Razor/Parser/ConditionalAttributeCollapser.cs @@ -20,7 +20,21 @@ namespace Microsoft.AspNet.Razor.Parser protected override bool CanRewrite(Block block) { var gen = block.ChunkGenerator as AttributeBlockChunkGenerator; - return gen != null && block.Children.Any() && block.Children.All(IsLiteralAttributeValue); + if (gen != null && block.Children.Count > 0) + { + // Perf: Avoid allocating an enumerator. + for (var i = 0; i < block.Children.Count; i++) + { + if (!IsLiteralAttributeValue(block.Children[i])) + { + return false; + } + } + + return true; + } + + return false; } protected override SyntaxTreeNode RewriteBlock(BlockBuilder parent, Block block) diff --git a/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs b/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs index 5f37760426..3aefbd2465 100644 --- a/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs +++ b/src/Microsoft.AspNet.Razor/Parser/ParserContext.cs @@ -253,7 +253,7 @@ namespace Microsoft.AspNet.Razor.Parser } return new ParserResults(_blockStack.Pop().Build(), - // TagHelperDescriptors are not found by default. The RazorParser is responsible + // TagHelperDescriptors are not found by default. The RazorParser is responsible // for identifying TagHelperDescriptors and rebuilding ParserResults. tagHelperDescriptors: Enumerable.Empty(), errorSink: _errorSink); diff --git a/src/Microsoft.AspNet.Razor/Parser/ParserVisitor.cs b/src/Microsoft.AspNet.Razor/Parser/ParserVisitor.cs index 8dff6cd68a..e950273e2f 100644 --- a/src/Microsoft.AspNet.Razor/Parser/ParserVisitor.cs +++ b/src/Microsoft.AspNet.Razor/Parser/ParserVisitor.cs @@ -14,9 +14,11 @@ namespace Microsoft.AspNet.Razor.Parser public virtual void VisitBlock(Block block) { VisitStartBlock(block); - foreach (SyntaxTreeNode node in block.Children) + + // Perf: Avoid allocating an enumerator. + for (var i = 0; i < block.Children.Count; i++) { - node.Accept(this); + block.Children[i].Accept(this); } VisitEndBlock(block); } diff --git a/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/Block.cs b/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/Block.cs index 6fa179ced1..395816401d 100644 --- a/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/Block.cs +++ b/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/Block.cs @@ -19,7 +19,7 @@ namespace Microsoft.AspNet.Razor.Parser.SyntaxTree source.Reset(); } - protected Block(BlockType? type, IEnumerable contents, IParentChunkGenerator generator) + protected Block(BlockType? type, IReadOnlyList contents, IParentChunkGenerator generator) { if (type == null) { @@ -30,14 +30,15 @@ namespace Microsoft.AspNet.Razor.Parser.SyntaxTree Children = contents; ChunkGenerator = generator; - foreach (SyntaxTreeNode node in Children) + // Perf: Avoid allocating an enumerator. + for (var i = 0; i < Children.Count; i++) { - node.Parent = this; + Children[i].Parent = this; } } // A Test constructor - internal Block(BlockType type, IEnumerable contents, IParentChunkGenerator generator) + internal Block(BlockType type, IReadOnlyList contents, IParentChunkGenerator generator) { Type = type; ChunkGenerator = generator; @@ -46,7 +47,7 @@ namespace Microsoft.AspNet.Razor.Parser.SyntaxTree public BlockType Type { get; } - public IEnumerable Children { get; } + public IReadOnlyList Children { get; } public IParentChunkGenerator ChunkGenerator { get; } @@ -133,9 +134,10 @@ namespace Microsoft.AspNet.Razor.Parser.SyntaxTree public virtual IEnumerable Flatten() { - // Create an enumerable that flattens the tree for use by syntax highlighters, etc. - foreach (SyntaxTreeNode element in Children) + // Perf: Avoid allocating an enumerator. + for (var i = 0; i < Children.Count; i++) { + var element = Children[i]; var span = element as Span; if (span != null) { diff --git a/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/BlockBuilder.cs b/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/BlockBuilder.cs index 852f628874..bff26cb3c0 100644 --- a/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/BlockBuilder.cs +++ b/src/Microsoft.AspNet.Razor/Parser/SyntaxTree/BlockBuilder.cs @@ -21,7 +21,7 @@ namespace Microsoft.AspNet.Razor.Parser.SyntaxTree } public BlockType? Type { get; set; } - public IList Children { get; private set; } + public List Children { get; private set; } public IParentChunkGenerator ChunkGenerator { get; set; } public virtual Block Build() diff --git a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs index f9704e3441..b6f996f6c0 100644 --- a/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs +++ b/src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperParseTreeRewriter.cs @@ -302,8 +302,19 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal private IEnumerable GetAttributeNames(Block tagBlock) { // Need to calculate how many children we should take that represent the attributes. - var childrenOffset = IsPartialTag(tagBlock) ? 1 : 2; - var attributeChildren = tagBlock.Children.Skip(1).Take(tagBlock.Children.Count() - childrenOffset); + var childrenOffset = IsPartialTag(tagBlock) ? 0 : 1; + var childCount = tagBlock.Children.Count - childrenOffset; + + if (childCount <= 1) + { + return Enumerable.Empty(); + } + + var attributeChildren = new List(childCount - 1); + for (var i = 1; i < childCount; i++) + { + attributeChildren.Add(tagBlock.Children[i]); + } var attributeNames = new List(); foreach (var child in attributeChildren) @@ -637,9 +648,9 @@ namespace Microsoft.AspNet.Razor.Parser.TagHelpers.Internal private static string GetTagName(Block tagBlock) { - var child = tagBlock.Children.First(); + var child = tagBlock.Children[0]; - if (tagBlock.Type != BlockType.Tag || !tagBlock.Children.Any() || !(child is Span)) + if (tagBlock.Type != BlockType.Tag || tagBlock.Children.Count == 0|| !(child is Span)) { return null; } diff --git a/test/Microsoft.AspNet.Razor.Test/Framework/BlockTypes.cs b/test/Microsoft.AspNet.Razor.Test/Framework/BlockTypes.cs index 37b2994059..f525f864d1 100644 --- a/test/Microsoft.AspNet.Razor.Test/Framework/BlockTypes.cs +++ b/test/Microsoft.AspNet.Razor.Test/Framework/BlockTypes.cs @@ -15,13 +15,13 @@ namespace Microsoft.AspNet.Razor.Test.Framework { private const BlockType ThisBlockType = BlockType.Statement; - public StatementBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public StatementBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : base(ThisBlockType, children, chunkGenerator) { } public StatementBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -29,24 +29,19 @@ namespace Microsoft.AspNet.Razor.Test.Framework : this(ParentChunkGenerator.Null, children) { } - - public StatementBlock(IEnumerable children) - : this(ParentChunkGenerator.Null, children) - { - } } public class DirectiveBlock : Block { private const BlockType ThisBlockType = BlockType.Directive; - public DirectiveBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public DirectiveBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : base(ThisBlockType, children, chunkGenerator) { } public DirectiveBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -54,24 +49,19 @@ namespace Microsoft.AspNet.Razor.Test.Framework : this(ParentChunkGenerator.Null, children) { } - - public DirectiveBlock(IEnumerable children) - : this(ParentChunkGenerator.Null, children) - { - } } public class FunctionsBlock : Block { private const BlockType ThisBlockType = BlockType.Functions; - public FunctionsBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public FunctionsBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : base(ThisBlockType, children, chunkGenerator) { } public FunctionsBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -79,24 +69,19 @@ namespace Microsoft.AspNet.Razor.Test.Framework : this(ParentChunkGenerator.Null, children) { } - - public FunctionsBlock(IEnumerable children) - : this(ParentChunkGenerator.Null, children) - { - } } public class ExpressionBlock : Block { private const BlockType ThisBlockType = BlockType.Expression; - public ExpressionBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public ExpressionBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : base(ThisBlockType, children, chunkGenerator) { } public ExpressionBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -104,11 +89,6 @@ namespace Microsoft.AspNet.Razor.Test.Framework : this(new ExpressionChunkGenerator(), children) { } - - public ExpressionBlock(IEnumerable children) - : this(new ExpressionChunkGenerator(), children) - { - } } public class MarkupTagBlock : Block @@ -128,18 +108,18 @@ namespace Microsoft.AspNet.Razor.Test.Framework public MarkupBlock( BlockType blockType, IParentChunkGenerator chunkGenerator, - IEnumerable children) + IReadOnlyList children) : base(blockType, children, chunkGenerator) { } - public MarkupBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public MarkupBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : this(ThisBlockType, chunkGenerator, children) { } public MarkupBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -147,11 +127,6 @@ namespace Microsoft.AspNet.Razor.Test.Framework : this(ParentChunkGenerator.Null, children) { } - - public MarkupBlock(IEnumerable children) - : this(ParentChunkGenerator.Null, children) - { - } } public class MarkupTagHelperBlock : TagHelperBlock @@ -221,13 +196,13 @@ namespace Microsoft.AspNet.Razor.Test.Framework { private const BlockType ThisBlockType = BlockType.Section; - public SectionBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public SectionBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : base(ThisBlockType, children, chunkGenerator) { } public SectionBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -236,7 +211,7 @@ namespace Microsoft.AspNet.Razor.Test.Framework { } - public SectionBlock(IEnumerable children) + public SectionBlock(IReadOnlyList children) : this(ParentChunkGenerator.Null, children) { } @@ -246,13 +221,13 @@ namespace Microsoft.AspNet.Razor.Test.Framework { private const BlockType ThisBlockType = BlockType.Template; - public TemplateBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public TemplateBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : base(ThisBlockType, children, chunkGenerator) { } public TemplateBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -261,7 +236,7 @@ namespace Microsoft.AspNet.Razor.Test.Framework { } - public TemplateBlock(IEnumerable children) + public TemplateBlock(IReadOnlyList children) : this(new TemplateBlockChunkGenerator(), children) { } @@ -271,13 +246,13 @@ namespace Microsoft.AspNet.Razor.Test.Framework { private const BlockType ThisBlockType = BlockType.Comment; - public CommentBlock(IParentChunkGenerator chunkGenerator, IEnumerable children) + public CommentBlock(IParentChunkGenerator chunkGenerator, IReadOnlyList children) : base(ThisBlockType, children, chunkGenerator) { } public CommentBlock(IParentChunkGenerator chunkGenerator, params SyntaxTreeNode[] children) - : this(chunkGenerator, (IEnumerable)children) + : this(chunkGenerator, (IReadOnlyList)children) { } @@ -286,7 +261,7 @@ namespace Microsoft.AspNet.Razor.Test.Framework { } - public CommentBlock(IEnumerable children) + public CommentBlock(IReadOnlyList children) : this(new RazorCommentChunkGenerator(), children) { } diff --git a/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs b/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs index cf7aa6479b..2cb05a91c2 100644 --- a/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs +++ b/test/Microsoft.AspNet.Razor.Test/Framework/ParserTestBase.cs @@ -531,7 +531,7 @@ namespace Microsoft.AspNet.Razor.Test.Framework private class IgnoreOutputBlock : Block { - public IgnoreOutputBlock() : base(BlockType.Template, Enumerable.Empty(), null) { } + public IgnoreOutputBlock() : base(BlockType.Template, new SyntaxTreeNode[0], null) { } } } }