Fix #6102 - Intense CPU utilization on page change

The issue here was that every time a Razor Page changed, we would
subscribe an additional time to the endpoint change notifications. This
means that if you tweaked a page 30 times, we would update the address
table 31 times when you save the file. If you were doing a lot of editing
then this would grow to a really large amount of computation.

The fix is to use DataSourceDependentCache, which is an existing utility
type we developed for this purpose. I'm not sure why it wasn't being
used for this already. We're already using DataSourceDependentCache in a
bunch of other places, and it's well tested.

I also tweaked the stucture of this code to be more similar to
EndpointNameAddressScheme. This involved some test changes that all
seemed like good cleanup. The way this was being tested was a little
wonky.

src/Http/Routing/src/DataSourceDependentCache.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// 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;
@@ -26,6 +26,11 @@ public DataSourceDependentCache(EndpointDataSource dataSource, Func<IReadOnlyLis
                 throw new ArgumentNullException(nameof(dataSource));
             }
 
+            if (initialize == null)
+            {
+                throw new ArgumentNullException(nameof(initialize));
+            }
+
             _dataSource = dataSource;
             _initializeCore = initialize;
 

src/Http/Routing/src/RouteValuesAddressScheme.cs

@@ -1,46 +1,44 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// 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.Linq;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Routing.Internal;
 using Microsoft.AspNetCore.Routing.Template;
 using Microsoft.AspNetCore.Routing.Tree;
-using Microsoft.Extensions.Primitives;
+using System;
+using System.Collections.Generic;
 
 namespace Microsoft.AspNetCore.Routing
 {
     internal class RouteValuesAddressScheme : IEndpointAddressScheme<RouteValuesAddress>
     {
-        private readonly EndpointDataSource _dataSource;
-        private LinkGenerationDecisionTree _allMatchesLinkGenerationTree;
-        private Dictionary<string, List<OutboundMatchResult>> _namedMatchResults;
-
+        private readonly DataSourceDependentCache<StateEntry> _cache;
+
         public RouteValuesAddressScheme(EndpointDataSource dataSource)
         {
-            _dataSource = dataSource;
-
-            // Build initial matches
-            BuildOutboundMatches();
-
-            // Register for changes in endpoints
-            ChangeToken.OnChange(
-                _dataSource.GetChangeToken,
-                HandleChange);
+            _cache = new DataSourceDependentCache<StateEntry>(dataSource, Initialize);
         }
 
+        // Internal for tests
+        internal StateEntry State => _cache.EnsureInitialized();
+
         public IEnumerable<Endpoint> FindEndpoints(RouteValuesAddress address)
         {
+            if (address == null)
+            {
+                throw new ArgumentNullException(nameof(address));
+            }
+
+            var state = State;
+
             IList<OutboundMatchResult> matchResults = null;
             if (string.IsNullOrEmpty(address.RouteName))
             {
-                matchResults = _allMatchesLinkGenerationTree.GetMatches(
+                matchResults = state.AllMatchesLinkGenerationTree.GetMatches(
                     address.ExplicitValues,
                     address.AmbientValues);
             }
-            else if (_namedMatchResults.TryGetValue(address.RouteName, out var namedMatchResults))
+            else if (state.NamedMatches.TryGetValue(address.RouteName, out var namedMatchResults))
             {
                 matchResults = namedMatchResults;
             }
@@ -74,52 +72,31 @@ private static IEnumerable<Endpoint> GetEndpoints(IList<OutboundMatchResult> mat
             }
         }
 
-        private void HandleChange()
-        {
-            // rebuild the matches
-            BuildOutboundMatches();
-
-            // re-register the callback as the change token is one time use only and a new change token
-            // is produced every time
-            ChangeToken.OnChange(
-                _dataSource.GetChangeToken,
-                HandleChange);
-        }
-
-        private void BuildOutboundMatches()
-        {
-            // Refresh the matches in the case where a datasource's endpoints changes. The following is OK to do
-            // 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);
-        }
-
-        /// Decision tree is built using the 'required values' of actions.
-        /// - When generating a url using route values, decision tree checks the explicitly supplied route values +
-        ///   ambient values to see if they have a match for the required-values-based-tree.
-        /// - When generating a url using route name, route values for controller, action etc.might not be provided
-        ///   (this is expected because as a user I want to avoid writing all those and instead chose to use a
-        ///   routename which is quick). So since these values are not provided and might not be even in ambient
-        ///   values, decision tree would fail to find a match. So for this reason decision tree is not used for named
-        ///   matches. Instead all named matches are returned as is and the LinkGenerator uses a TemplateBinder to
-        ///   decide which of the matches can generate a url.
-        ///   For example, for a route defined like below with current ambient values like new { controller = "Home",
-        ///   action = "Index" }
-        ///     "api/orders/{id}",
-        ///     routeName: "OrdersApi",
-        ///     defaults: new { controller = "Orders", action = "GetById" },
-        ///     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 (List<OutboundMatch>, Dictionary<string, List<OutboundMatchResult>>) GetOutboundMatches()
+        private StateEntry Initialize(IReadOnlyList<Endpoint> endpoints)
         {
             var allOutboundMatches = new List<OutboundMatch>();
-            var namedOutboundMatchResults = new Dictionary<string, List<OutboundMatchResult>>(
-                StringComparer.OrdinalIgnoreCase);
-
-            foreach (var endpoint in _dataSource.Endpoints)
+            var namedOutboundMatchResults = new Dictionary<string, List<OutboundMatchResult>>(StringComparer.OrdinalIgnoreCase);
+
+            // Decision tree is built using the 'required values' of actions.
+            // - When generating a url using route values, decision tree checks the explicitly supplied route values +
+            //   ambient values to see if they have a match for the required-values-based-tree.
+            // - When generating a url using route name, route values for controller, action etc.might not be provided
+            //   (this is expected because as a user I want to avoid writing all those and instead chose to use a
+            //   routename which is quick). So since these values are not provided and might not be even in ambient
+            //   values, decision tree would fail to find a match. So for this reason decision tree is not used for named
+            //   matches. Instead all named matches are returned as is and the LinkGenerator uses a TemplateBinder to
+            //   decide which of the matches can generate a url.
+            //   For example, for a route defined like below with current ambient values like new { controller = "Home",
+            //   action = "Index" }
+            //     "api/orders/{id}",
+            //     routeName: "OrdersApi",
+            //     defaults: new { controller = "Orders", action = "GetById" },
+            //     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.
+            for (var i = 0; i < endpoints.Count; i++)
             {
+                var endpoint = endpoints[i];
                 if (!(endpoint is RouteEndpoint routeEndpoint))
                 {
                     continue;
@@ -157,7 +134,10 @@ protected virtual (List<OutboundMatch>, Dictionary<string, List<OutboundMatchRes
                 matchResults.Add(new OutboundMatchResult(outboundMatch, isFallbackMatch: false));
             }
 
-            return (allOutboundMatches, namedOutboundMatchResults);
+            return new StateEntry(
+                allOutboundMatches,
+                new LinkGenerationDecisionTree(allOutboundMatches),
+                namedOutboundMatchResults);
         }
 
         private OutboundRouteEntry CreateOutboundRouteEntry(
@@ -178,5 +158,23 @@ private OutboundRouteEntry CreateOutboundRouteEntry(
             entry.Defaults = new RouteValueDictionary(endpoint.RoutePattern.Defaults);
             return entry;
         }
+
+        internal class StateEntry
+        {
+            // For testing
+            public readonly List<OutboundMatch> AllMatches;
+            public readonly LinkGenerationDecisionTree AllMatchesLinkGenerationTree;
+            public readonly Dictionary<string, List<OutboundMatchResult>> NamedMatches;
+
+            public StateEntry(
+                List<OutboundMatch> allMatches,
+                LinkGenerationDecisionTree allMatchesLinkGenerationTree,
+                Dictionary<string, List<OutboundMatchResult>> namedMatches)
+            {
+                AllMatches = allMatches;
+                AllMatchesLinkGenerationTree = allMatchesLinkGenerationTree;
+                NamedMatches = namedMatches;
+            }
+        }
     }
 }

src/Http/Routing/test/UnitTests/RouteValuesAddressSchemeTest.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// 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.Collections.Generic;
@@ -25,10 +25,10 @@ public void GetOutboundMatches_GetsNamedMatchesFor_EndpointsHaving_IRouteNameMet
             var addressScheme = CreateAddressScheme(endpoint1, endpoint2);
 
             // Assert
-            Assert.NotNull(addressScheme.AllMatches);
-            Assert.Equal(2, addressScheme.AllMatches.Count());
-            Assert.NotNull(addressScheme.NamedMatches);
-            Assert.True(addressScheme.NamedMatches.TryGetValue("named", out var namedMatches));
+            Assert.NotNull(addressScheme.State.AllMatches);
+            Assert.Equal(2, addressScheme.State.AllMatches.Count());
+            Assert.NotNull(addressScheme.State.NamedMatches);
+            Assert.True(addressScheme.State.NamedMatches.TryGetValue("named", out var namedMatches));
             var namedMatch = Assert.Single(namedMatches);
             var actual = Assert.IsType<RouteEndpoint>(namedMatch.Match.Entry.Data);
             Assert.Same(endpoint2, actual);
@@ -46,10 +46,10 @@ public void GetOutboundMatches_GroupsMultipleEndpoints_WithSameName()
             var addressScheme = CreateAddressScheme(endpoint1, endpoint2, endpoint3);
 
             // Assert
-            Assert.NotNull(addressScheme.AllMatches);
-            Assert.Equal(3, addressScheme.AllMatches.Count());
-            Assert.NotNull(addressScheme.NamedMatches);
-            Assert.True(addressScheme.NamedMatches.TryGetValue("named", out var namedMatches));
+            Assert.NotNull(addressScheme.State.AllMatches);
+            Assert.Equal(3, addressScheme.State.AllMatches.Count());
+            Assert.NotNull(addressScheme.State.NamedMatches);
+            Assert.True(addressScheme.State.NamedMatches.TryGetValue("named", out var namedMatches));
             Assert.Equal(2, namedMatches.Count);
             Assert.Same(endpoint2, Assert.IsType<RouteEndpoint>(namedMatches[0].Match.Entry.Data));
             Assert.Same(endpoint3, Assert.IsType<RouteEndpoint>(namedMatches[1].Match.Entry.Data));
@@ -67,10 +67,10 @@ public void GetOutboundMatches_GroupsMultipleEndpoints_WithSameName_IgnoringCase
             var addressScheme = CreateAddressScheme(endpoint1, endpoint2, endpoint3);
 
             // Assert
-            Assert.NotNull(addressScheme.AllMatches);
-            Assert.Equal(3, addressScheme.AllMatches.Count());
-            Assert.NotNull(addressScheme.NamedMatches);
-            Assert.True(addressScheme.NamedMatches.TryGetValue("named", out var namedMatches));
+            Assert.NotNull(addressScheme.State.AllMatches);
+            Assert.Equal(3, addressScheme.State.AllMatches.Count());
+            Assert.NotNull(addressScheme.State.NamedMatches);
+            Assert.True(addressScheme.State.NamedMatches.TryGetValue("named", out var namedMatches));
             Assert.Equal(2, namedMatches.Count);
             Assert.Same(endpoint2, Assert.IsType<RouteEndpoint>(namedMatches[0].Match.Entry.Data));
             Assert.Same(endpoint3, Assert.IsType<RouteEndpoint>(namedMatches[1].Match.Entry.Data));
@@ -84,11 +84,12 @@ public void EndpointDataSource_ChangeCallback_Refreshes_OutboundMatches()
             var dynamicDataSource = new DynamicEndpointDataSource(new[] { endpoint1 });
 
             // Act 1
-            var addressScheme = new CustomRouteValuesBasedAddressScheme(new CompositeEndpointDataSource(new[] { dynamicDataSource }));
+            var addressScheme = new RouteValuesAddressScheme(new CompositeEndpointDataSource(new[] { dynamicDataSource }));
 
             // Assert 1
-            Assert.NotNull(addressScheme.AllMatches);
-            var match = Assert.Single(addressScheme.AllMatches);
+            var state = addressScheme.State;
+            Assert.NotNull(state.AllMatches);
+            var match = Assert.Single(state.AllMatches);
             var actual = Assert.IsType<RouteEndpoint>(match.Entry.Data);
             Assert.Same(endpoint1, actual);
 
@@ -99,24 +100,35 @@ public void EndpointDataSource_ChangeCallback_Refreshes_OutboundMatches()
             // Trigger change
             dynamicDataSource.AddEndpoint(endpoint2);
 
-            // Arrange 2
+            // Assert 2
+            Assert.NotSame(state, addressScheme.State);
+            state = addressScheme.State;
+
+            // Arrange 3
             var endpoint3 = CreateEndpoint("/c", routeName: "c");
 
-            // Act 2
+            // Act 3
             // Trigger change
             dynamicDataSource.AddEndpoint(endpoint3);
 
-            // Arrange 3
+            // Assert 3
+            Assert.NotSame(state, addressScheme.State);
+            state = addressScheme.State;
+
+            // Arrange 4
             var endpoint4 = CreateEndpoint("/d", routeName: "d");
 
-            // Act 3
+            // Act 4
             // Trigger change
             dynamicDataSource.AddEndpoint(endpoint4);
 
-            // Assert 3
-            Assert.NotNull(addressScheme.AllMatches);
+            // Assert 4
+            Assert.NotSame(state, addressScheme.State);
+            state = addressScheme.State;
+
+            Assert.NotNull(state.AllMatches);
             Assert.Collection(
-                addressScheme.AllMatches,
+                state.AllMatches,
                 (m) =>
                 {
                     actual = Assert.IsType<RouteEndpoint>(m.Entry.Data);
@@ -337,7 +349,7 @@ public void GetOutboundMatches_DoesNotInclude_EndpointsWithSuppressLinkGeneratio
             var addressScheme = CreateAddressScheme(endpoint);
 
             // Assert
-            Assert.Empty(addressScheme.AllMatches);
+            Assert.Empty(addressScheme.State.AllMatches);
         }
 
         [Fact]
@@ -352,17 +364,17 @@ public void AddressScheme_UnsuppressedEndpoint_IsUsed()
             var addressScheme = CreateAddressScheme(endpoint);
 
             // Assert
-            Assert.Same(endpoint, Assert.Single(addressScheme.AllMatches).Entry.Data);
+            Assert.Same(endpoint, Assert.Single(addressScheme.State.AllMatches).Entry.Data);
         }
 
-        private CustomRouteValuesBasedAddressScheme CreateAddressScheme(params Endpoint[] endpoints)
+        private RouteValuesAddressScheme CreateAddressScheme(params Endpoint[] endpoints)
         {
             return CreateAddressScheme(new DefaultEndpointDataSource(endpoints));
         }
 
-        private CustomRouteValuesBasedAddressScheme CreateAddressScheme(params EndpointDataSource[] dataSources)
+        private RouteValuesAddressScheme CreateAddressScheme(params EndpointDataSource[] dataSources)
         {
-            return new CustomRouteValuesBasedAddressScheme(new CompositeEndpointDataSource(dataSources));
+            return new RouteValuesAddressScheme(new CompositeEndpointDataSource(dataSources));
         }
 
         private RouteEndpoint CreateEndpoint(
@@ -391,26 +403,6 @@ private RouteEndpoint CreateEndpoint(
                 null);
         }
 
-        private class CustomRouteValuesBasedAddressScheme : RouteValuesAddressScheme
-        {
-            public CustomRouteValuesBasedAddressScheme(CompositeEndpointDataSource dataSource)
-                : base(dataSource)
-            {
-            }
-
-            public IEnumerable<OutboundMatch> AllMatches { get; private set; }
-
-            public IDictionary<string, List<OutboundMatchResult>> NamedMatches { get; private set; }
-
-            protected override (List<OutboundMatch>, Dictionary<string, List<OutboundMatchResult>>) GetOutboundMatches()
-            {
-                var matches = base.GetOutboundMatches();
-                AllMatches = matches.Item1;
-                NamedMatches = matches.Item2;
-                return matches;
-            }
-        }
-
         private class EncourageLinkGenerationMetadata : ISuppressLinkGenerationMetadata
         {
             public bool SuppressLinkGeneration => false;