From 0f90a15cf1982fbe968f12e30750e3a6d5a5fe95 Mon Sep 17 00:00:00 2001 From: Gert Driesen Date: Sun, 21 Oct 2018 21:34:11 +0200 Subject: [PATCH 1/2] Use Array.Clone() to copy arrays, and avoid copy where applicable (#855) --- .../Patterns/RoutePatternFactory.cs | 22 +- .../Patterns/RoutePatternFactoryTest.cs | 235 +++++++++++++++++- 2 files changed, 244 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.AspNetCore.Routing/Patterns/RoutePatternFactory.cs b/src/Microsoft.AspNetCore.Routing/Patterns/RoutePatternFactory.cs index 71941ffde6..2f755a08e9 100644 --- a/src/Microsoft.AspNetCore.Routing/Patterns/RoutePatternFactory.cs +++ b/src/Microsoft.AspNetCore.Routing/Patterns/RoutePatternFactory.cs @@ -2,12 +2,10 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Linq; using Microsoft.AspNetCore.Routing.Constraints; -using Microsoft.AspNetCore.Routing.Matching; namespace Microsoft.AspNetCore.Routing.Patterns { @@ -417,7 +415,7 @@ namespace Microsoft.AspNetCore.Routing.Patterns parameter.Name, @default, parameter.ParameterKind, - (IEnumerable)parameterConstraints ?? Array.Empty(), + parameterConstraints?.ToArray() ?? Array.Empty(), parameter.EncodeSlashes); } } @@ -435,7 +433,7 @@ namespace Microsoft.AspNetCore.Routing.Patterns throw new ArgumentNullException(nameof(parts)); } - return SegmentCore(parts); + return SegmentCore(parts.ToArray()); } /// @@ -451,12 +449,12 @@ namespace Microsoft.AspNetCore.Routing.Patterns throw new ArgumentNullException(nameof(parts)); } - return SegmentCore(parts); + return SegmentCore((RoutePatternPart[]) parts.Clone()); } - private static RoutePatternPathSegment SegmentCore(IEnumerable parts) + private static RoutePatternPathSegment SegmentCore(RoutePatternPart[] parts) { - return new RoutePatternPathSegment(parts.ToArray()); + return new RoutePatternPathSegment(parts); } /// @@ -630,7 +628,7 @@ namespace Microsoft.AspNetCore.Routing.Patterns parameterName: parameterName, @default: @default, parameterKind: parameterKind, - parameterPolicies: parameterPolicies); + parameterPolicies: parameterPolicies.ToArray()); } /// @@ -672,14 +670,14 @@ namespace Microsoft.AspNetCore.Routing.Patterns parameterName: parameterName, @default: @default, parameterKind: parameterKind, - parameterPolicies: parameterPolicies); + parameterPolicies: (RoutePatternParameterPolicyReference[]) parameterPolicies.Clone()); } private static RoutePatternParameterPart ParameterPartCore( string parameterName, object @default, RoutePatternParameterKind parameterKind, - IEnumerable parameterPolicies) + RoutePatternParameterPolicyReference[] parameterPolicies) { return ParameterPartCore(parameterName, @default, parameterKind, parameterPolicies, encodeSlashes: true); } @@ -688,14 +686,14 @@ namespace Microsoft.AspNetCore.Routing.Patterns string parameterName, object @default, RoutePatternParameterKind parameterKind, - IEnumerable parameterPolicies, + RoutePatternParameterPolicyReference[] parameterPolicies, bool encodeSlashes) { return new RoutePatternParameterPart( parameterName, @default, parameterKind, - parameterPolicies.ToArray(), + parameterPolicies, encodeSlashes); } diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Patterns/RoutePatternFactoryTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Patterns/RoutePatternFactoryTest.cs index 50d22c6a22..b56465dd7a 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Patterns/RoutePatternFactoryTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Patterns/RoutePatternFactoryTest.cs @@ -2,9 +2,10 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; using System.Linq; using Microsoft.AspNetCore.Routing.Constraints; -using Microsoft.AspNetCore.Routing.Matching; +using Microsoft.AspNetCore.Routing.Template; using Moq; using Xunit; @@ -303,5 +304,237 @@ namespace Microsoft.AspNetCore.Routing.Patterns $"Invalid constraint '17'. A constraint must be of type 'string' or '{typeof(IRouteConstraint)}'.", ex.Message); } + + [Fact] + public void Pattern_ArrayOfSegments_ShouldMakeCopyOfArrayOfSegments() + { + // Arrange + var literalPartA = RoutePatternFactory.LiteralPart("A"); + var paramPartB = RoutePatternFactory.ParameterPart("B"); + var paramPartC = RoutePatternFactory.ParameterPart("C"); + var paramPartD = RoutePatternFactory.ParameterPart("D"); + var segments = new[] + { + RoutePatternFactory.Segment(literalPartA, paramPartB), + RoutePatternFactory.Segment(paramPartC, literalPartA), + RoutePatternFactory.Segment(paramPartD), + RoutePatternFactory.Segment(literalPartA) + }; + + // Act + var actual = RoutePatternFactory.Pattern(segments); + segments[1] = RoutePatternFactory.Segment(RoutePatternFactory.ParameterPart("E")); + Array.Resize(ref segments, 2); + + // Assert + Assert.Equal(3, actual.Parameters.Count); + Assert.Same(paramPartB, actual.Parameters[0]); + Assert.Same(paramPartC, actual.Parameters[1]); + Assert.Same(paramPartD, actual.Parameters[2]); + } + + [Fact] + public void Pattern_RawTextAndArrayOfSegments_ShouldMakeCopyOfArrayOfSegments() + { + // Arrange + var rawText = "raw"; + var literalPartA = RoutePatternFactory.LiteralPart("A"); + var paramPartB = RoutePatternFactory.ParameterPart("B"); + var paramPartC = RoutePatternFactory.ParameterPart("C"); + var paramPartD = RoutePatternFactory.ParameterPart("D"); + var segments = new[] + { + RoutePatternFactory.Segment(literalPartA, paramPartB), + RoutePatternFactory.Segment(paramPartC, literalPartA), + RoutePatternFactory.Segment(paramPartD), + RoutePatternFactory.Segment(literalPartA) + }; + + // Act + var actual = RoutePatternFactory.Pattern(rawText, segments); + segments[1] = RoutePatternFactory.Segment(RoutePatternFactory.ParameterPart("E")); + Array.Resize(ref segments, 2); + + // Assert + Assert.Equal(3, actual.Parameters.Count); + Assert.Same(paramPartB, actual.Parameters[0]); + Assert.Same(paramPartC, actual.Parameters[1]); + Assert.Same(paramPartD, actual.Parameters[2]); + } + + [Fact] + public void Pattern_DefaultsAndParameterPoliciesAndArrayOfSegments_ShouldMakeCopyOfArrayOfSegments() + { + // Arrange + object defaults = new { B = 12, C = 4 }; + object parameterPolicies = null; + var literalPartA = RoutePatternFactory.LiteralPart("A"); + var paramPartB = RoutePatternFactory.ParameterPart("B"); + var paramPartC = RoutePatternFactory.ParameterPart("C"); + var paramPartD = RoutePatternFactory.ParameterPart("D"); + var segments = new[] + { + RoutePatternFactory.Segment(literalPartA, paramPartB), + RoutePatternFactory.Segment(paramPartC, literalPartA), + RoutePatternFactory.Segment(paramPartD), + RoutePatternFactory.Segment(literalPartA) + }; + + // Act + var actual = RoutePatternFactory.Pattern(defaults, parameterPolicies, segments); + segments[1] = RoutePatternFactory.Segment(RoutePatternFactory.ParameterPart("E")); + Array.Resize(ref segments, 2); + + // Assert + Assert.Equal(3, actual.Parameters.Count); + Assert.Equal(paramPartB.Name, actual.Parameters[0].Name); + Assert.Equal(12, actual.Parameters[0].Default); + Assert.Null(paramPartB.Default); + Assert.NotSame(paramPartB, actual.Parameters[0]); + Assert.Equal(paramPartC.Name, actual.Parameters[1].Name); + Assert.Equal(4, actual.Parameters[1].Default); + Assert.NotSame(paramPartC, actual.Parameters[1]); + Assert.Null(paramPartC.Default); + Assert.Equal(paramPartD.Name, actual.Parameters[2].Name); + Assert.Null(actual.Parameters[2].Default); + Assert.Same(paramPartD, actual.Parameters[2]); + Assert.Null(paramPartD.Default); + } + + [Fact] + public void Pattern_RawTextAndDefaultsAndParameterPoliciesAndArrayOfSegments_ShouldMakeCopyOfArrayOfSegments() + { + // Arrange + var rawText = "raw"; + object defaults = new { B = 12, C = 4 }; + object parameterPolicies = null; + var literalPartA = RoutePatternFactory.LiteralPart("A"); + var paramPartB = RoutePatternFactory.ParameterPart("B"); + var paramPartC = RoutePatternFactory.ParameterPart("C"); + var paramPartD = RoutePatternFactory.ParameterPart("D"); + var segments = new[] + { + RoutePatternFactory.Segment(literalPartA, paramPartB), + RoutePatternFactory.Segment(paramPartC, literalPartA), + RoutePatternFactory.Segment(paramPartD), + RoutePatternFactory.Segment(literalPartA) + }; + + // Act + var actual = RoutePatternFactory.Pattern(rawText, defaults, parameterPolicies, segments); + segments[1] = RoutePatternFactory.Segment(RoutePatternFactory.ParameterPart("E")); + Array.Resize(ref segments, 2); + + // Assert + Assert.Equal(3, actual.Parameters.Count); + Assert.Equal(paramPartB.Name, actual.Parameters[0].Name); + Assert.Equal(12, actual.Parameters[0].Default); + Assert.Null(paramPartB.Default); + Assert.NotSame(paramPartB, actual.Parameters[0]); + Assert.Equal(paramPartC.Name, actual.Parameters[1].Name); + Assert.Equal(4, actual.Parameters[1].Default); + Assert.NotSame(paramPartC, actual.Parameters[1]); + Assert.Null(paramPartC.Default); + Assert.Equal(paramPartD.Name, actual.Parameters[2].Name); + Assert.Null(actual.Parameters[2].Default); + Assert.Same(paramPartD, actual.Parameters[2]); + Assert.Null(paramPartD.Default); + } + + [Fact] + public void ParameterPart_ParameterNameAndDefaultAndParameterKindAndArrayOfParameterPolicies_ShouldMakeCopyOfParameterPolicies() + { + // Arrange (going through hoops to get an array of RoutePatternParameterPolicyReference) + const string name = "Id"; + var defaults = new { a = "13", }; + var x = new InlineConstraint("x"); + var y = new InlineConstraint("y"); + var z = new InlineConstraint("z"); + var constraints = new[] { x, y, z }; + var templatePart = TemplatePart.CreateParameter("t", false, false, null, constraints); + var routePatternParameterPart = (RoutePatternParameterPart) templatePart.ToRoutePatternPart(); + var policies = routePatternParameterPart.ParameterPolicies.ToArray(); + + // Act + var parameterPart = RoutePatternFactory.ParameterPart(name, defaults, RoutePatternParameterKind.Standard, policies); + policies[0] = null; + Array.Resize(ref policies, 2); + + // Assert + Assert.NotNull(parameterPart.ParameterPolicies); + Assert.Equal(3, parameterPart.ParameterPolicies.Count); + Assert.NotNull(parameterPart.ParameterPolicies[0]); + Assert.NotNull(parameterPart.ParameterPolicies[1]); + Assert.NotNull(parameterPart.ParameterPolicies[2]); + } + + [Fact] + public void ParameterPart_ParameterNameAndDefaultAndParameterKindAndEnumerableOfParameterPolicies_ShouldMakeCopyOfParameterPolicies() + { + // Arrange (going through hoops to get an enumerable of RoutePatternParameterPolicyReference) + const string name = "Id"; + var defaults = new { a = "13", }; + var x = new InlineConstraint("x"); + var y = new InlineConstraint("y"); + var z = new InlineConstraint("z"); + var constraints = new[] { x, y, z }; + var templatePart = TemplatePart.CreateParameter("t", false, false, null, constraints); + var routePatternParameterPart = (RoutePatternParameterPart)templatePart.ToRoutePatternPart(); + var policies = routePatternParameterPart.ParameterPolicies.ToList(); + + // Act + var parameterPart = RoutePatternFactory.ParameterPart(name, defaults, RoutePatternParameterKind.Standard, policies); + policies[0] = null; + policies.RemoveAt(1); + + // Assert + Assert.NotNull(parameterPart.ParameterPolicies); + Assert.Equal(3, parameterPart.ParameterPolicies.Count); + Assert.NotNull(parameterPart.ParameterPolicies[0]); + Assert.NotNull(parameterPart.ParameterPolicies[1]); + Assert.NotNull(parameterPart.ParameterPolicies[2]); + } + + [Fact] + public void Segment_EnumerableOfParts() + { + // Arrange + var paramPartB = RoutePatternFactory.ParameterPart("B"); + var paramPartC = RoutePatternFactory.ParameterPart("C"); + var paramPartD = RoutePatternFactory.ParameterPart("D"); + var parts = new[] { paramPartB, paramPartC, paramPartD }; + + // Act + var actual = RoutePatternFactory.Segment((IEnumerable) parts); + parts[1] = RoutePatternFactory.ParameterPart("E"); + Array.Resize(ref parts, 2); + + // Assert + Assert.Equal(3, actual.Parts.Count); + Assert.Same(paramPartB, actual.Parts[0]); + Assert.Same(paramPartC, actual.Parts[1]); + Assert.Same(paramPartD, actual.Parts[2]); + } + + [Fact] + public void Segment_ArrayOfParts() + { + // Arrange + var paramPartB = RoutePatternFactory.ParameterPart("B"); + var paramPartC = RoutePatternFactory.ParameterPart("C"); + var paramPartD = RoutePatternFactory.ParameterPart("D"); + var parts = new[] { paramPartB, paramPartC, paramPartD }; + + // Act + var actual = RoutePatternFactory.Segment(parts); + parts[1] = RoutePatternFactory.ParameterPart("E"); + Array.Resize(ref parts, 2); + + // Assert + Assert.Equal(3, actual.Parts.Count); + Assert.Same(paramPartB, actual.Parts[0]); + Assert.Same(paramPartC, actual.Parts[1]); + Assert.Same(paramPartD, actual.Parts[2]); + } } } From 2081160678e787eb9312a1ed15789f393f2058e6 Mon Sep 17 00:00:00 2001 From: Gert Driesen Date: Sun, 21 Oct 2018 21:35:35 +0200 Subject: [PATCH 2/2] Improve performance and reduce allocations of TemplateSegment (#856) --- .../Template/TemplateSegment.cs | 13 ++++- .../Template/TemplateSegmentTest.cs | 49 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 test/Microsoft.AspNetCore.Routing.Tests/Template/TemplateSegmentTest.cs diff --git a/src/Microsoft.AspNetCore.Routing/Template/TemplateSegment.cs b/src/Microsoft.AspNetCore.Routing/Template/TemplateSegment.cs index a3aec42686..304472653e 100644 --- a/src/Microsoft.AspNetCore.Routing/Template/TemplateSegment.cs +++ b/src/Microsoft.AspNetCore.Routing/Template/TemplateSegment.cs @@ -1,6 +1,7 @@ // 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; using System.Collections.Generic; using System.Diagnostics; using System.Linq; @@ -18,7 +19,17 @@ namespace Microsoft.AspNetCore.Routing.Template public TemplateSegment(RoutePatternPathSegment other) { - Parts = new List(other.Parts.Select(s => new TemplatePart(s))); + if (other == null) + { + throw new ArgumentNullException(nameof(other)); + } + + var partCount = other.Parts.Count; + Parts = new List(partCount); + for (var i = 0; i < partCount; i++) + { + Parts.Add(new TemplatePart(other.Parts[i])); + } } public bool IsSimple => Parts.Count == 1; diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Template/TemplateSegmentTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Template/TemplateSegmentTest.cs new file mode 100644 index 0000000000..62f56f20b7 --- /dev/null +++ b/test/Microsoft.AspNetCore.Routing.Tests/Template/TemplateSegmentTest.cs @@ -0,0 +1,49 @@ +// 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; +using Microsoft.AspNetCore.Routing.Patterns; +using Xunit; + +namespace Microsoft.AspNetCore.Routing.Template +{ + public class TemplateSegmentTest + { + [Fact] + public void Ctor_RoutePatternPathSegment_ShouldThrowArgumentNullExceptionWhenOtherIsNull() + { + const RoutePatternPathSegment other = null; + + var actual = Assert.ThrowsAny(() => new TemplateSegment(other)); + Assert.Equal(nameof(other), actual.ParamName); + } + + [Fact] + public void ToRoutePatternPathSegment() + { + // Arrange + var literalPartA = RoutePatternFactory.LiteralPart("A"); + var paramPartB = RoutePatternFactory.ParameterPart("B"); + var paramPartC = RoutePatternFactory.ParameterPart("C"); + var paramPartD = RoutePatternFactory.ParameterPart("D"); + var separatorPartE = RoutePatternFactory.SeparatorPart("E"); + var templateSegment = new TemplateSegment(RoutePatternFactory.Segment(paramPartC, literalPartA, separatorPartE, paramPartB)); + + // Act + var routePatternPathSegment = templateSegment.ToRoutePatternPathSegment(); + templateSegment.Parts[1] = new TemplatePart(RoutePatternFactory.ParameterPart("D")); + templateSegment.Parts.RemoveAt(0); + + // Assert + Assert.Equal(4, routePatternPathSegment.Parts.Count); + Assert.IsType(routePatternPathSegment.Parts[0]); + Assert.Equal(paramPartC.Name, ((RoutePatternParameterPart) routePatternPathSegment.Parts[0]).Name); + Assert.IsType(routePatternPathSegment.Parts[1]); + Assert.Equal(literalPartA.Content, ((RoutePatternLiteralPart) routePatternPathSegment.Parts[1]).Content); + Assert.IsType(routePatternPathSegment.Parts[2]); + Assert.Equal(separatorPartE.Content, ((RoutePatternSeparatorPart) routePatternPathSegment.Parts[2]).Content); + Assert.IsType(routePatternPathSegment.Parts[3]); + Assert.Equal(paramPartB.Name, ((RoutePatternParameterPart) routePatternPathSegment.Parts[3]).Name); + } + } +}