From bc482cd2b093e417e65f9baa86882024976d9c33 Mon Sep 17 00:00:00 2001 From: Gert Driesen Date: Tue, 23 Oct 2018 02:25:08 +0200 Subject: [PATCH 1/3] Minor performance improvement for DfaMatcherBuilder (#854) --- .../MatcherBuilderMultipleEntryBenchmark.cs | 158 ++++++++++++++++++ .../Matching/DfaMatcherBuilder.cs | 84 +++++++--- 2 files changed, 219 insertions(+), 23 deletions(-) diff --git a/benchmarks/Microsoft.AspNetCore.Routing.Performance/Matching/MatcherBuilderMultipleEntryBenchmark.cs b/benchmarks/Microsoft.AspNetCore.Routing.Performance/Matching/MatcherBuilderMultipleEntryBenchmark.cs index 002118caca..23f1bf0729 100644 --- a/benchmarks/Microsoft.AspNetCore.Routing.Performance/Matching/MatcherBuilderMultipleEntryBenchmark.cs +++ b/benchmarks/Microsoft.AspNetCore.Routing.Performance/Matching/MatcherBuilderMultipleEntryBenchmark.cs @@ -2,14 +2,25 @@ // 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.Threading.Tasks; using BenchmarkDotNet.Attributes; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing.TestObjects; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Routing.Matching { public partial class MatcherBuilderMultipleEntryBenchmark : EndpointRoutingBenchmarkBase { private IServiceProvider _services; + private List _policies; + private ILoggerFactory _loggerFactory; + private DefaultEndpointSelector _selector; + private DefaultParameterPolicyFactory _parameterPolicyFactory; [GlobalSetup] public void Setup() @@ -28,6 +39,26 @@ namespace Microsoft.AspNetCore.Routing.Matching Endpoints[8] = CreateEndpoint("/v2/account/{id}", "POST"); Endpoints[9] = CreateEndpoint("/v2/account/{id}", "UPDATE"); + // Define an unordered mixture of policies that implement INodeBuilderPolicy, + // IEndpointComparerPolicy and/or IEndpointSelectorPolicy + _policies = new List() + { + CreateNodeBuilderPolicy(4), + CreateUberPolicy(2), + CreateNodeBuilderPolicy(3), + CreateEndpointComparerPolicy(5), + CreateNodeBuilderPolicy(1), + CreateEndpointSelectorPolicy(9), + CreateEndpointComparerPolicy(7), + CreateNodeBuilderPolicy(6), + CreateEndpointSelectorPolicy(10), + CreateUberPolicy(12), + CreateEndpointComparerPolicy(11) + }; + _loggerFactory = NullLoggerFactory.Instance; + _selector = new DefaultEndpointSelector(); + _parameterPolicyFactory = new DefaultParameterPolicyFactory(Options.Create(new RouteOptions()), new TestServiceProvider()); + _services = CreateServices(); } @@ -46,5 +77,132 @@ namespace Microsoft.AspNetCore.Routing.Matching var builder = _services.GetRequiredService(); SetupMatcher(builder); } + + [Benchmark] + public void Constructor_Policies() + { + new DfaMatcherBuilder(_loggerFactory, _parameterPolicyFactory, _selector, _policies); + } + + private static MatcherPolicy CreateNodeBuilderPolicy(int order) + { + return new TestNodeBuilderPolicy(order); + } + private static MatcherPolicy CreateEndpointComparerPolicy(int order) + { + return new TestEndpointComparerPolicy(order); + } + + private static MatcherPolicy CreateEndpointSelectorPolicy(int order) + { + return new TestEndpointSelectorPolicy(order); + } + + private static MatcherPolicy CreateUberPolicy(int order) + { + return new TestUberPolicy(order); + } + + private class TestUberPolicy : TestMatcherPolicyBase, INodeBuilderPolicy, IEndpointComparerPolicy + { + public TestUberPolicy(int order) : base(order) + { + } + + public IComparer Comparer => new TestEndpointComparer(); + + public bool AppliesToEndpoints(IReadOnlyList endpoints) + { + return false; + } + + public PolicyJumpTable BuildJumpTable(int exitDestination, IReadOnlyList edges) + { + throw new NotImplementedException(); + } + + public IReadOnlyList GetEdges(IReadOnlyList endpoints) + { + throw new NotImplementedException(); + } + } + + private class TestNodeBuilderPolicy : TestMatcherPolicyBase, INodeBuilderPolicy + { + public TestNodeBuilderPolicy(int order) : base(order) + { + } + + public bool AppliesToEndpoints(IReadOnlyList endpoints) + { + return false; + } + + public PolicyJumpTable BuildJumpTable(int exitDestination, IReadOnlyList edges) + { + throw new NotImplementedException(); + } + + public IReadOnlyList GetEdges(IReadOnlyList endpoints) + { + throw new NotImplementedException(); + } + } + + private class TestEndpointComparerPolicy : TestMatcherPolicyBase, IEndpointComparerPolicy + { + public TestEndpointComparerPolicy(int order) : base(order) + { + } + + public IComparer Comparer => new TestEndpointComparer(); + + public bool AppliesToEndpoints(IReadOnlyList endpoints) + { + return false; + } + + public Task ApplyAsync(HttpContext httpContext, EndpointSelectorContext context, CandidateSet candidates) + { + throw new NotImplementedException(); + } + } + + private class TestEndpointSelectorPolicy : TestMatcherPolicyBase, IEndpointSelectorPolicy + { + public TestEndpointSelectorPolicy(int order) : base(order) + { + } + + public bool AppliesToEndpoints(IReadOnlyList endpoints) + { + return false; + } + + public Task ApplyAsync(HttpContext httpContext, EndpointSelectorContext context, CandidateSet candidates) + { + throw new NotImplementedException(); + } + } + + private abstract class TestMatcherPolicyBase : MatcherPolicy + { + private int _order; + + protected TestMatcherPolicyBase(int order) + { + _order = order; + } + + public override int Order { get { return _order; } } + } + + private class TestEndpointComparer : IComparer + { + public int Compare(Endpoint x, Endpoint y) + { + return 0; + } + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Routing/Matching/DfaMatcherBuilder.cs b/src/Microsoft.AspNetCore.Routing/Matching/DfaMatcherBuilder.cs index b4e9ff538d..1b2172db53 100644 --- a/src/Microsoft.AspNetCore.Routing/Matching/DfaMatcherBuilder.cs +++ b/src/Microsoft.AspNetCore.Routing/Matching/DfaMatcherBuilder.cs @@ -17,7 +17,7 @@ namespace Microsoft.AspNetCore.Routing.Matching private readonly ILoggerFactory _loggerFactory; private readonly ParameterPolicyFactory _parameterPolicyFactory; private readonly EndpointSelector _selector; - private readonly MatcherPolicy[] _policies; + private readonly IEndpointSelectorPolicy[] _endpointSelectorPolicies; private readonly INodeBuilderPolicy[] _nodeBuilders; private readonly EndpointComparer _comparer; @@ -39,11 +39,11 @@ namespace Microsoft.AspNetCore.Routing.Matching _loggerFactory = loggerFactory; _parameterPolicyFactory = parameterPolicyFactory; _selector = selector; - _policies = policies.OrderBy(p => p.Order).ToArray(); - // Taking care to use _policies, which has been sorted. - _nodeBuilders = _policies.OfType().ToArray(); - _comparer = new EndpointComparer(_policies.OfType().ToArray()); + var (nodeBuilderPolicies, endpointComparerPolicies, endpointSelectorPolicies) = ExtractPolicies(policies.OrderBy(p => p.Order)); + _endpointSelectorPolicies = endpointSelectorPolicies; + _nodeBuilders = nodeBuilderPolicies; + _comparer = new EndpointComparer(endpointComparerPolicies); _assignments = new Dictionary(StringComparer.OrdinalIgnoreCase); _slots = new List>(); @@ -383,10 +383,10 @@ namespace Microsoft.AspNetCore.Routing.Matching List endpointSelectorPolicies = null; if (node.Matches?.Count > 0) { - for (var i = 0; i < _policies.Length; i++) + for (var i = 0; i < _endpointSelectorPolicies.Length; i++) { - if (_policies[i] is IEndpointSelectorPolicy endpointSelectorPolicy && - endpointSelectorPolicy.AppliesToEndpoints(node.Matches)) + var endpointSelectorPolicy = _endpointSelectorPolicies[i]; + if (endpointSelectorPolicy.AppliesToEndpoints(node.Matches)) { if (endpointSelectorPolicies == null) { @@ -465,16 +465,16 @@ namespace Microsoft.AspNetCore.Routing.Matching // internal for tests internal Candidate CreateCandidate(Endpoint endpoint, int score) { - _assignments.Clear(); - _slots.Clear(); - _captures.Clear(); - _complexSegments.Clear(); - _constraints.Clear(); - (string parameterName, int segmentIndex, int slotIndex) catchAll = default; if (endpoint is RouteEndpoint routeEndpoint) { + _assignments.Clear(); + _slots.Clear(); + _captures.Clear(); + _complexSegments.Clear(); + _constraints.Clear(); + foreach (var kvp in routeEndpoint.RoutePattern.Defaults) { _assignments.Add(kvp.Key, _assignments.Count); @@ -539,16 +539,27 @@ namespace Microsoft.AspNetCore.Routing.Matching } } } - } - return new Candidate( - endpoint, - score, - _slots.ToArray(), - _captures.ToArray(), - catchAll, - _complexSegments.ToArray(), - _constraints.ToArray()); + return new Candidate( + endpoint, + score, + _slots.ToArray(), + _captures.ToArray(), + catchAll, + _complexSegments.ToArray(), + _constraints.ToArray()); + } + else + { + return new Candidate( + endpoint, + score, + Array.Empty>(), + Array.Empty<(string parameterName, int segmentIndex, int slotIndex)>(), + catchAll, + Array.Empty<(RoutePatternPathSegment pathSegment, int segmentIndex)>(), + Array.Empty>()); + } } private int[] GetGroupLengths(DfaNode node) @@ -682,5 +693,32 @@ namespace Microsoft.AspNetCore.Routing.Matching work = nextWork; } } + + private static (INodeBuilderPolicy[] nodeBuilderPolicies, IEndpointComparerPolicy[] endpointComparerPolicies, IEndpointSelectorPolicy[] endpointSelectorPolicies) ExtractPolicies(IEnumerable policies) + { + var nodeBuilderPolicies = new List(); + var endpointComparerPolicies = new List(); + var endpointSelectorPolicies = new List(); + + foreach (var policy in policies) + { + if (policy is INodeBuilderPolicy nodeBuilderPolicy) + { + nodeBuilderPolicies.Add(nodeBuilderPolicy); + } + + if (policy is IEndpointComparerPolicy endpointComparerPolicy) + { + endpointComparerPolicies.Add(endpointComparerPolicy); + } + + if (policy is IEndpointSelectorPolicy endpointSelectorPolicy) + { + endpointSelectorPolicies.Add(endpointSelectorPolicy); + } + } + + return (nodeBuilderPolicies.ToArray(), endpointComparerPolicies.ToArray(), endpointSelectorPolicies.ToArray()); + } } } From 25b5ab2c3980569907d7c467dff185d00b9fbd64 Mon Sep 17 00:00:00 2001 From: Gert Driesen Date: Tue, 23 Oct 2018 02:25:43 +0200 Subject: [PATCH 2/3] Improve performance and reduce allocations in RouteValuesAddressScheme. (#879) --- .../RouteValuesAddressScheme.cs | 50 ++++++++--- .../RouteValuesAddressSchemeTest.cs | 90 ++++++++++++++++++- 2 files changed, 125 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.AspNetCore.Routing/RouteValuesAddressScheme.cs b/src/Microsoft.AspNetCore.Routing/RouteValuesAddressScheme.cs index dc103128d1..ae7bd66705 100644 --- a/src/Microsoft.AspNetCore.Routing/RouteValuesAddressScheme.cs +++ b/src/Microsoft.AspNetCore.Routing/RouteValuesAddressScheme.cs @@ -16,7 +16,7 @@ namespace Microsoft.AspNetCore.Routing { private readonly CompositeEndpointDataSource _dataSource; private LinkGenerationDecisionTree _allMatchesLinkGenerationTree; - private IDictionary> _namedMatchResults; + private Dictionary> _namedMatchResults; public RouteValuesAddressScheme(CompositeEndpointDataSource dataSource) { @@ -33,7 +33,7 @@ namespace Microsoft.AspNetCore.Routing public IEnumerable FindEndpoints(RouteValuesAddress address) { - IEnumerable matchResults = null; + IList matchResults = null; if (string.IsNullOrEmpty(address.RouteName)) { matchResults = _allMatchesLinkGenerationTree.GetMatches( @@ -45,14 +45,33 @@ namespace Microsoft.AspNetCore.Routing matchResults = namedMatchResults; } - if (matchResults == null || !matchResults.Any()) + if (matchResults != null) { - return Array.Empty(); + var matchCount = matchResults.Count; + if (matchCount > 0) + { + if (matchResults.Count == 1) + { + // Special case having a single result to avoid creating iterator state machine + return new[] { (RouteEndpoint)matchResults[0].Match.Entry.Data }; + } + else + { + // Use separate method since one cannot have regular returns in an iterator method + return GetEndpoints(matchResults, matchCount); + } + } } - return matchResults - .Select(matchResult => matchResult.Match) - .Select(match => (RouteEndpoint)match.Entry.Data); + return Array.Empty(); + } + + private static IEnumerable GetEndpoints(IList matchResults, int matchCount) + { + for (var i = 0; i < matchCount; i++) + { + yield return (RouteEndpoint)matchResults[i].Match.Entry.Data; + } } private void HandleChange() @@ -73,7 +92,7 @@ namespace Microsoft.AspNetCore.Routing // as refresh of new endpoints happens within a lock and also these fields are not publicly accessible. var (allMatches, namedMatchResults) = GetOutboundMatches(); _namedMatchResults = namedMatchResults; - _allMatchesLinkGenerationTree = new LinkGenerationDecisionTree(allMatches.ToArray()); + _allMatchesLinkGenerationTree = new LinkGenerationDecisionTree(allMatches); } /// Decision tree is built using the 'required values' of actions. @@ -93,21 +112,25 @@ namespace Microsoft.AspNetCore.Routing /// requiredValues: new { controller = "Orders", action = "GetById" }, /// A call to GetLink("OrdersApi", new { id = "10" }) cannot generate url as neither the supplied values or /// current ambient values do not satisfy the decision tree that is built based on the required values. - protected virtual (IEnumerable, IDictionary>) GetOutboundMatches() + protected virtual (List, Dictionary>) GetOutboundMatches() { var allOutboundMatches = new List(); var namedOutboundMatchResults = new Dictionary>( StringComparer.OrdinalIgnoreCase); - var endpoints = _dataSource.Endpoints.OfType(); - foreach (var endpoint in endpoints) + foreach (var endpoint in _dataSource.Endpoints) { + if (!(endpoint is RouteEndpoint routeEndpoint)) + { + continue; + } + if (endpoint.Metadata.GetMetadata()?.SuppressLinkGeneration == true) { continue; } - var entry = CreateOutboundRouteEntry(endpoint); + var entry = CreateOutboundRouteEntry(routeEndpoint); var outboundMatch = new OutboundMatch() { Entry = entry }; allOutboundMatches.Add(outboundMatch); @@ -117,8 +140,7 @@ namespace Microsoft.AspNetCore.Routing continue; } - List matchResults; - if (!namedOutboundMatchResults.TryGetValue(entry.RouteName, out matchResults)) + if (!namedOutboundMatchResults.TryGetValue(entry.RouteName, out var matchResults)) { matchResults = new List(); namedOutboundMatchResults.Add(entry.RouteName, matchResults); diff --git a/test/Microsoft.AspNetCore.Routing.Tests/RouteValuesAddressSchemeTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/RouteValuesAddressSchemeTest.cs index 9020939218..fc92eeab27 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/RouteValuesAddressSchemeTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/RouteValuesAddressSchemeTest.cs @@ -139,6 +139,94 @@ namespace Microsoft.AspNetCore.Routing }); } + [Fact] + public void FindEndpoints_LookedUpByCriteria_NoMatch() + { + // Arrange + var endpoint1 = CreateEndpoint( + "api/orders/{id}/{name?}/{urgent=true}/{zipCode}", + defaults: new { zipCode = 3510 }, + requiredValues: new { id = 7 }, + routeName: "OrdersApi"); + var endpoint2 = CreateEndpoint( + "api/orders/{id}/{name?}/{urgent=true}/{zipCode}", + defaults: new { id = 12 }, + requiredValues: new { zipCode = 3510 }, + routeName: "OrdersApi"); + var addressScheme = CreateAddressScheme(endpoint1, endpoint2); + + // Act + var foundEndpoints = addressScheme.FindEndpoints( + new RouteValuesAddress + { + ExplicitValues = new RouteValueDictionary(new { id = 8 }), + AmbientValues = new RouteValueDictionary(new { urgent = false }), + }); + + // Assert + Assert.Empty(foundEndpoints); + } + + [Fact] + public void FindEndpoints_LookedUpByCriteria_OneMatch() + { + // Arrange + var endpoint1 = CreateEndpoint( + "api/orders/{id}/{name?}/{urgent=true}/{zipCode}", + defaults: new { zipCode = 3510 }, + requiredValues: new { id = 7 }, + routeName: "OrdersApi"); + var endpoint2 = CreateEndpoint( + "api/orders/{id}/{name?}/{urgent=true}/{zipCode}", + defaults: new { id = 12 }, + routeName: "OrdersApi"); + var addressScheme = CreateAddressScheme(endpoint1, endpoint2); + + // Act + var foundEndpoints = addressScheme.FindEndpoints( + new RouteValuesAddress + { + ExplicitValues = new RouteValueDictionary(new { id = 13 }), + AmbientValues = new RouteValueDictionary(new { zipCode = 3500 }), + }); + + // Assert + var actual = Assert.Single(foundEndpoints); + Assert.Same(endpoint2, actual); + } + + [Fact] + public void FindEndpoints_LookedUpByCriteria_MultipleMatches() + { + // Arrange + var endpoint1 = CreateEndpoint( + "api/orders/{id}/{name?}/{urgent=true}/{zipCode}", + defaults: new { zipCode = 3510 }, + requiredValues: new { id = 7 }, + routeName: "OrdersApi"); + var endpoint2 = CreateEndpoint( + "api/orders/{id}/{name?}/{urgent}/{zipCode}", + defaults: new { id = 12 }, + routeName: "OrdersApi"); + var endpoint3 = CreateEndpoint( + "api/orders/{id}/{name?}/{urgent=true}/{zipCode}", + defaults: new { id = 12 }, + routeName: "OrdersApi"); + var addressScheme = CreateAddressScheme(endpoint1, endpoint2, endpoint3); + + // Act + var foundEndpoints = addressScheme.FindEndpoints( + new RouteValuesAddress + { + ExplicitValues = new RouteValueDictionary(new { id = 7 }), + AmbientValues = new RouteValueDictionary(new { zipCode = 3500 }), + }); + + // Assert + Assert.Contains(endpoint1, foundEndpoints); + Assert.Contains(endpoint1, foundEndpoints); + } + [Fact] public void FindEndpoints_ReturnsEndpoint_WhenLookedUpByRouteName() { @@ -270,7 +358,7 @@ namespace Microsoft.AspNetCore.Routing public IDictionary> NamedMatches { get; private set; } - protected override (IEnumerable, IDictionary>) GetOutboundMatches() + protected override (List, Dictionary>) GetOutboundMatches() { var matches = base.GetOutboundMatches(); AllMatches = matches.Item1; From bd32ec38375263f6b1a77a1a0757c032d8f0f2b3 Mon Sep 17 00:00:00 2001 From: Gert Driesen Date: Tue, 23 Oct 2018 02:26:17 +0200 Subject: [PATCH 3/3] Add basic test for RouteValueEqualityComparer.Equals(...). (#883) --- .../RouteValueEqualityComparerTest.cs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 test/Microsoft.AspNetCore.Routing.Tests/RouteValueEqualityComparerTest.cs diff --git a/test/Microsoft.AspNetCore.Routing.Tests/RouteValueEqualityComparerTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/RouteValueEqualityComparerTest.cs new file mode 100644 index 0000000000..4f6dc0a87d --- /dev/null +++ b/test/Microsoft.AspNetCore.Routing.Tests/RouteValueEqualityComparerTest.cs @@ -0,0 +1,41 @@ +// 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 Xunit; + +namespace Microsoft.AspNetCore.Routing +{ + public class RouteValueEqualityComparerTest + { + private readonly RouteValueEqualityComparer _comparer; + + public RouteValueEqualityComparerTest() + { + _comparer = new RouteValueEqualityComparer(); + } + + [Theory] + [InlineData(5, 7, false)] + [InlineData("foo", "foo", true)] + [InlineData("foo", "FoO", true)] + [InlineData("foo", "boo", false)] + [InlineData("7", 7, true)] + [InlineData(7, "7", true)] + [InlineData(5.7d, 5.7d, true)] + [InlineData(null, null, true)] + [InlineData(null, "foo", false)] + [InlineData("foo", null, false)] + [InlineData(null, "", true)] + [InlineData("", null, true)] + [InlineData("", "", true)] + [InlineData("", "foo", false)] + [InlineData("foo", "", false)] + [InlineData(true, true, true)] + [InlineData(true, false, false)] + public void EqualsTest(object x, object y, bool expected) + { + var actual = _comparer.Equals(x, y); + Assert.Equal(expected, actual); + } + } +}