diff --git a/src/Microsoft.AspNetCore.Routing.Abstractions/RouteData.cs b/src/Microsoft.AspNetCore.Routing.Abstractions/RouteData.cs index 2d2131c8d7..620dccb701 100644 --- a/src/Microsoft.AspNetCore.Routing.Abstractions/RouteData.cs +++ b/src/Microsoft.AspNetCore.Routing.Abstractions/RouteData.cs @@ -12,6 +12,7 @@ namespace Microsoft.AspNetCore.Routing public class RouteData { private RouteValueDictionary _dataTokens; + private IList _routers; private RouteValueDictionary _values; /// @@ -19,8 +20,7 @@ namespace Microsoft.AspNetCore.Routing /// public RouteData() { - // Perf: Avoid allocating DataTokens and RouteValues unless needed. - Routers = new List(); + // Perf: Avoid allocating collections unless needed. } /// @@ -34,9 +34,12 @@ namespace Microsoft.AspNetCore.Routing throw new ArgumentNullException(nameof(other)); } - Routers = new List(other.Routers); - - // Perf: Avoid allocating DataTokens and RouteValues unless we need to make a copy. + // Perf: Avoid allocating collections unless we need to make a copy. + if (other._routers != null) + { + _routers = new List(other.Routers); + } + if (other._dataTokens != null) { _dataTokens = new RouteValueDictionary(other._dataTokens); @@ -67,7 +70,18 @@ namespace Microsoft.AspNetCore.Routing /// /// Gets the list of instances on the current routing path. /// - public List Routers { get; } + public IList Routers + { + get + { + if (_routers == null) + { + _routers = new List(); + } + + return _routers; + } + } /// /// Gets the set of values produced by routes on the current routing path. @@ -84,5 +98,160 @@ namespace Microsoft.AspNetCore.Routing return _values; } } + + /// + /// + /// Creates a snapshot of the current state of the before appending + /// to , merging into + /// , and merging into . + /// + /// + /// Call to restore the state of this + /// to the state at the time of calling + /// . + /// + /// + /// + /// An to append to . If null, then + /// will not be changed. + /// + /// + /// A to merge into . If null, then + /// will not be changed. + /// + /// + /// A to merge into . If null, then + /// will not be changed. + /// + /// A that captures the current state. + public RouteDataSnapshot PushState(IRouter router, RouteValueDictionary values, RouteValueDictionary dataTokens) + { + var snapshot = new RouteDataSnapshot( + this, + _dataTokens?.Count > 0 ? new RouteValueDictionary(_dataTokens) : null, + _routers?.Count > 0 ? new List(_routers) : null, + _values?.Count > 0 ? new RouteValueDictionary(_values) : null); + + if (router != null) + { + Routers.Add(router); + } + + if (values != null) + { + foreach (var kvp in values) + { + if (kvp.Value != null) + { + Values[kvp.Key] = kvp.Value; + } + } + } + + if (dataTokens != null) + { + foreach (var kvp in dataTokens) + { + DataTokens[kvp.Key] = kvp.Value; + } + } + + return snapshot; + } + + /// + /// A snapshot of the state of a instance. + /// + public struct RouteDataSnapshot + { + private readonly RouteData _routeData; + private readonly RouteValueDictionary _dataTokens; + private readonly IList _routers; + private readonly RouteValueDictionary _values; + + /// + /// Creates a new for . + /// + /// The . + /// The data tokens. + /// The routers. + /// The route values. + public RouteDataSnapshot( + RouteData routeData, + RouteValueDictionary dataTokens, + IList routers, + RouteValueDictionary values) + { + if (routeData == null) + { + throw new ArgumentNullException(nameof(routeData)); + } + + _routeData = routeData; + _dataTokens = dataTokens; + _routers = routers; + _values = values; + } + + /// + /// Restores the to the captured state. + /// + public void Restore() + { + if (_routeData._dataTokens == null && _dataTokens == null) + { + // Do nothing + } + else if (_dataTokens == null) + { + _routeData._dataTokens.Clear(); + } + else + { + _routeData._dataTokens.Clear(); + + foreach (var kvp in _dataTokens) + { + _routeData._dataTokens.Add(kvp.Key, kvp.Value); + } + } + + if (_routeData._routers == null && _routers == null) + { + // Do nothing + } + else if (_routers == null) + { + _routeData._routers.Clear(); + } + else + { + _routeData._routers.Clear(); + + for (var i = 0; i < _routers.Count; i++) + { + _routeData._routers.Add(_routers[i]); + } + } + + if (_routeData._values == null && _values == null) + { + // Do nothing + } + else if (_values == null) + { + _routeData._values.Clear(); + } + else + { + _routeData._values.Clear(); + + foreach (var kvp in _values) + { + _routeData._values.Add(kvp.Key, kvp.Value); + } + } + } + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Routing/RouteBase.cs b/src/Microsoft.AspNetCore.Routing/RouteBase.cs index 5dd5ef3411..98a0fc3340 100644 --- a/src/Microsoft.AspNetCore.Routing/RouteBase.cs +++ b/src/Microsoft.AspNetCore.Routing/RouteBase.cs @@ -19,6 +19,12 @@ namespace Microsoft.AspNetCore.Routing { private TemplateMatcher _matcher; private TemplateBinder _binder; + private readonly IReadOnlyDictionary _constraints; + private readonly RouteValueDictionary _dataTokens; + private readonly RouteValueDictionary _defaults; + private readonly IRouter _target; + private readonly RouteTemplate _parsedTemplate; + private readonly string _routeTemplate; private ILogger _logger; private ILogger _constraintLogger; diff --git a/src/Microsoft.AspNetCore.Routing/RouteCollection.cs b/src/Microsoft.AspNetCore.Routing/RouteCollection.cs index 98a41320bd..d0be724b5b 100644 --- a/src/Microsoft.AspNetCore.Routing/RouteCollection.cs +++ b/src/Microsoft.AspNetCore.Routing/RouteCollection.cs @@ -54,19 +54,18 @@ namespace Microsoft.AspNetCore.Routing public async virtual Task RouteAsync(RouteContext context) { + // Perf: We want to avoid allocating a new RouteData for each route we need to process. + // We can do this by snapshotting the state at the beginning and then restoring it + // for each router we execute. + var snapshot = context.RouteData.PushState(null, values: null, dataTokens: null); + for (var i = 0; i < Count; i++) { var route = this[i]; - - var oldRouteData = context.RouteData; - - var newRouteData = new RouteData(oldRouteData); - newRouteData.Routers.Add(route); + context.RouteData.Routers.Add(route); try { - context.RouteData = newRouteData; - await route.RouteAsync(context); if (context.Handler != null) @@ -78,7 +77,7 @@ namespace Microsoft.AspNetCore.Routing { if (context.Handler == null) { - context.RouteData = oldRouteData; + snapshot.Restore(); } } } diff --git a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs index 4fd5a28222..ae35f8e5d8 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs @@ -173,31 +173,23 @@ namespace Microsoft.AspNetCore.Routing.Tree } var match = new TemplateMatch(item, values); - - var oldRouteData = context.RouteData; - - var newRouteData = new RouteData(oldRouteData); - - newRouteData.Routers.Add(match.Entry.Target); - MergeValues(newRouteData.Values, match.Values); - - if (!RouteConstraintMatcher.Match( - match.Entry.Constraints, - newRouteData.Values, - context.HttpContext, - this, - RouteDirection.IncomingRequest, - _constraintLogger)) - { - continue; - } - - _logger.MatchedRoute(match.Entry.RouteName, match.Entry.RouteTemplate.TemplateText); - - context.RouteData = newRouteData; + var snapshot = context.RouteData.PushState(match.Entry.Target, match.Values, dataTokens: null); try { + if (!RouteConstraintMatcher.Match( + match.Entry.Constraints, + context.RouteData.Values, + context.HttpContext, + this, + RouteDirection.IncomingRequest, + _constraintLogger)) + { + continue; + } + + _logger.MatchedRoute(match.Entry.RouteName, match.Entry.RouteTemplate.TemplateText); + await match.Entry.Target.RouteAsync(context); if (context.Handler != null) { @@ -209,7 +201,7 @@ namespace Microsoft.AspNetCore.Routing.Tree if (context.Handler == null) { // Restore the original values to prevent polluting the route data. - context.RouteData = oldRouteData; + snapshot.Restore(); } } } @@ -308,22 +300,6 @@ namespace Microsoft.AspNetCore.Routing.Tree } } - private static void MergeValues( - RouteValueDictionary destination, - RouteValueDictionary values) - { - foreach (var kvp in values) - { - if (kvp.Value != null) - { - // This will replace the original value for the specified key. - // Values from the matched route will take preference over previous - // data in the route context. - destination[kvp.Key] = kvp.Value; - } - } - } - private struct TemplateMatch : IEquatable { public TemplateMatch(TreeRouteMatchingEntry entry, RouteValueDictionary values) diff --git a/test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteDataTest.cs b/test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteDataTest.cs new file mode 100644 index 0000000000..b8eac36c7e --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteDataTest.cs @@ -0,0 +1,157 @@ +// 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 Moq; +using Xunit; + +namespace Microsoft.AspNetCore.Routing +{ + public class RouteDataTest + { + [Fact] + public void RouteData_DefaultPropertyValues() + { + // Arrange & Act + var routeData = new RouteData(); + + // Assert + Assert.Empty(routeData.DataTokens); + Assert.Empty(routeData.Routers); + Assert.Empty(routeData.Values); + } + + [Fact] + public void RouteData_CopyConstructor() + { + // Arrange & Act + var original = new RouteData(); + + original.DataTokens.Add("data", "token"); + original.Routers.Add(Mock.Of()); + original.Values.Add("route", "value"); + + var routeData = new RouteData(original); + + // Assert + Assert.NotSame(routeData.DataTokens, original.DataTokens); + Assert.Equal(routeData.DataTokens, original.DataTokens); + Assert.NotSame(routeData.Routers, original.Routers); + Assert.Equal(routeData.Routers, original.Routers); + Assert.NotSame(routeData.Values, original.Values); + Assert.Equal(routeData.Values, original.Values); + } + + [Fact] + public void RouteData_PushStateAndRestore_NullValues() + { + // Arrange + var routeData = new RouteData(); + + // Act + var snapshot = routeData.PushState(null, null, null); + var copy = new RouteData(routeData); + snapshot.Restore(); + + // Assert + Assert.Equal(routeData.DataTokens, copy.DataTokens); + Assert.Equal(routeData.Routers, copy.Routers); + Assert.Equal(routeData.Values, copy.Values); + } + + [Fact] + public void RouteData_PushStateAndRestore_EmptyValues() + { + // Arrange + var routeData = new RouteData(); + + // Act + var snapshot = routeData.PushState(null, new RouteValueDictionary(), new RouteValueDictionary()); + var copy = new RouteData(routeData); + snapshot.Restore(); + + // Assert + Assert.Equal(routeData.DataTokens, copy.DataTokens); + Assert.Equal(routeData.Routers, copy.Routers); + Assert.Equal(routeData.Values, copy.Values); + } + + // This is an important semantic for catchall parameters. A null route value shouldn't be + // merged. + [Fact] + public void RouteData_PushStateAndRestore_NullRouteValueNotSet() + { + // Arrange + var original = new RouteData(); + original.Values.Add("bleh", "16"); + + var routeData = new RouteData(original); + + // Act + var snapshot = routeData.PushState( + null, + new RouteValueDictionary(new { bleh = (string)null }), + new RouteValueDictionary()); + snapshot.Restore(); + + // Assert + Assert.Equal(routeData.Values, original.Values); + } + + [Fact] + public void RouteData_PushStateAndThenModify() + { + // Arrange + var routeData = new RouteData(); + + // Act + var snapshot = routeData.PushState(null, null, null); + routeData.DataTokens.Add("data", "token"); + routeData.Routers.Add(Mock.Of()); + routeData.Values.Add("route", "value"); + + var copy = new RouteData(routeData); + snapshot.Restore(); + + // Assert + Assert.Empty(routeData.DataTokens); + Assert.NotEqual(routeData.DataTokens, copy.DataTokens); + Assert.Empty(routeData.Routers); + Assert.NotEqual(routeData.Routers, copy.Routers); + Assert.Empty(routeData.Values); + Assert.NotEqual(routeData.Values, copy.Values); + } + + [Fact] + public void RouteData_PushStateAndThenModify_WithInitialData() + { + // Arrange + var original = new RouteData(); + original.DataTokens.Add("data", "token1"); + original.Routers.Add(Mock.Of()); + original.Values.Add("route", "value1"); + + var routeData = new RouteData(original); + + // Act + var snapshot = routeData.PushState( + Mock.Of(), + new RouteValueDictionary(new { route = "value2" }), + new RouteValueDictionary(new { data = "token2" })); + + routeData.DataTokens.Add("data2", "token"); + routeData.Routers.Add(Mock.Of()); + routeData.Values.Add("route2", "value"); + + var copy = new RouteData(routeData); + snapshot.Restore(); + + // Assert + Assert.Equal(original.DataTokens, routeData.DataTokens); + Assert.NotEqual(routeData.DataTokens, copy.DataTokens); + Assert.Equal(original.Routers, routeData.Routers); + Assert.NotEqual(routeData.Routers, copy.Routers); + Assert.Equal(original.Values, routeData.Values); + Assert.NotEqual(routeData.Values, copy.Values); + } + } +} diff --git a/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs index 9ca95e5611..8fff1423d8 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs @@ -155,11 +155,9 @@ namespace Microsoft.AspNetCore.Routing Assert.Equal("USA", context.RouteData.Values["country"]); Assert.True(context.RouteData.Values.ContainsKey("id")); Assert.Equal("5", context.RouteData.Values["id"]); - Assert.Same(originalRouteDataValues, context.RouteData.Values); Assert.Equal("Contoso", context.RouteData.DataTokens["company"]); Assert.Equal("Friday", context.RouteData.DataTokens["today"]); - Assert.Same(originalDataTokens, context.RouteData.DataTokens); } [Fact] diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs index 37c741db79..30dd445490 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs @@ -1343,18 +1343,18 @@ namespace Microsoft.AspNetCore.Routing.Tree } [Fact] - public async Task TreeRouter_CreatesNewRouteData() + public async Task TreeRouter_SnapshotsRouteData() { // Arrange - RouteData nestedRouteData = null; + RouteValueDictionary nestedValues = null; var next = new Mock(); next .Setup(r => r.RouteAsync(It.IsAny())) .Callback(c => { - nestedRouteData = c.RouteData; - c.Handler = NullHandler; + nestedValues = new RouteValueDictionary(c.RouteData.Values); + c.Handler = null; // Not a match }) .Returns(Task.FromResult(true)) .Verifiable(); @@ -1364,23 +1364,17 @@ namespace Microsoft.AspNetCore.Routing.Tree var context = CreateRouteContext("/api/Store"); - var originalRouteData = context.RouteData; - originalRouteData.Values.Add("action", "Index"); + var routeData = context.RouteData; + routeData.Values.Add("action", "Index"); + + var originalValues = new RouteValueDictionary(context.RouteData.Values); // Act await route.RouteAsync(context); // Assert - Assert.NotSame(originalRouteData, context.RouteData); - Assert.NotSame(originalRouteData, nestedRouteData); - Assert.Same(nestedRouteData, context.RouteData); - - // The new routedata is a copy - Assert.Equal("Index", context.RouteData.Values["action"]); - Assert.Single(context.RouteData.Values, kvp => kvp.Key == "test_route_group"); - - Assert.Equal(1, context.RouteData.Routers.Count); - Assert.Equal(next.Object.GetType(), context.RouteData.Routers[0].GetType()); + Assert.Equal(originalValues, context.RouteData.Values); + Assert.NotEqual(nestedValues, context.RouteData.Values); } [Fact] @@ -1436,16 +1430,19 @@ namespace Microsoft.AspNetCore.Routing.Tree } [Fact] - public async Task TreeRouter_CreatesNewRouteData_ResetsWhenNotMatched() + public async Task TreeRouter_SnapshotsRouteData_ResetsWhenNotMatched() { // Arrange - RouteData nestedRouteData = null; + RouteValueDictionary nestedValues = null; + List nestedRouters = null; + var next = new Mock(); next .Setup(r => r.RouteAsync(It.IsAny())) .Callback(c => { - nestedRouteData = c.RouteData; + nestedValues = new RouteValueDictionary(c.RouteData.Values); + nestedRouters = new List(c.RouteData.Routers); }) .Returns(Task.FromResult(true)) .Verifiable(); @@ -1455,40 +1452,39 @@ namespace Microsoft.AspNetCore.Routing.Tree var context = CreateRouteContext("/api/Store"); - var originalRouteData = context.RouteData; - originalRouteData.Values.Add("action", "Index"); + context.RouteData.Values.Add("action", "Index"); // Act await route.RouteAsync(context); // Assert - Assert.Same(originalRouteData, context.RouteData); - Assert.NotSame(originalRouteData, nestedRouteData); - Assert.NotSame(nestedRouteData, context.RouteData); + Assert.NotEqual(nestedValues, context.RouteData.Values); // The new routedata is a copy Assert.Equal("Index", context.RouteData.Values["action"]); - Assert.Equal("Index", nestedRouteData.Values["action"]); + Assert.Equal("Index", nestedValues["action"]); Assert.DoesNotContain(context.RouteData.Values, kvp => kvp.Key == "test_route_group"); - Assert.Single(nestedRouteData.Values, kvp => kvp.Key == "test_route_group"); + Assert.Single(nestedValues, kvp => kvp.Key == "test_route_group"); Assert.Empty(context.RouteData.Routers); - Assert.Equal(1, nestedRouteData.Routers.Count); - Assert.Equal(next.Object.GetType(), nestedRouteData.Routers[0].GetType()); + Assert.Equal(1, nestedRouters.Count); + Assert.Equal(next.Object.GetType(), nestedRouters[0].GetType()); } [Fact] public async Task TreeRouter_CreatesNewRouteData_ResetsWhenThrows() { // Arrange - RouteData nestedRouteData = null; + RouteValueDictionary nestedValues = null; + List nestedRouters = null; var next = new Mock(); next .Setup(r => r.RouteAsync(It.IsAny())) .Callback(c => { - nestedRouteData = c.RouteData; + nestedValues = new RouteValueDictionary(c.RouteData.Values); + nestedRouters = new List(c.RouteData.Routers); }) .Throws(new Exception()); @@ -1496,28 +1492,24 @@ namespace Microsoft.AspNetCore.Routing.Tree var route = CreateAttributeRoute(next.Object, entry); var context = CreateRouteContext("/api/Store"); - - var originalRouteData = context.RouteData; - originalRouteData.Values.Add("action", "Index"); + context.RouteData.Values.Add("action", "Index"); // Act await Assert.ThrowsAsync(() => route.RouteAsync(context)); // Assert - Assert.Same(originalRouteData, context.RouteData); - Assert.NotSame(originalRouteData, nestedRouteData); - Assert.NotSame(nestedRouteData, context.RouteData); + Assert.NotEqual(nestedValues, context.RouteData.Values); // The new routedata is a copy Assert.Equal("Index", context.RouteData.Values["action"]); - Assert.Equal("Index", nestedRouteData.Values["action"]); + Assert.Equal("Index", nestedValues["action"]); Assert.DoesNotContain(context.RouteData.Values, kvp => kvp.Key == "test_route_group"); - Assert.Single(nestedRouteData.Values, kvp => kvp.Key == "test_route_group"); + Assert.Single(nestedValues, kvp => kvp.Key == "test_route_group"); Assert.Empty(context.RouteData.Routers); - Assert.Equal(1, nestedRouteData.Routers.Count); - Assert.Equal(next.Object.GetType(), nestedRouteData.Routers[0].GetType()); + Assert.Equal(1, nestedRouters.Count); + Assert.Equal(next.Object.GetType(), nestedRouters[0].GetType()); } private static RouteContext CreateRouteContext(string requestPath)