Fix for #4666 - reduce modelbinders created

This change to ModelBinderFactory makes the caching much more aggressive,
by caching all non-root binders. There's some trickiness here around
making sure we have the right behavior when all providers return null. See
the tests and comments.

I also kept the change I made for a temporary workaround to use a
dictionary rather than a "stack" for cycle breaking.  This seems like an
overall improvement in clarity.

src/Microsoft.AspNetCore.Mvc.Core/Internal/PlaceholderBinder.cs

@@ -2,8 +2,9 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Mvc.ModelBinding;
 
-namespace Microsoft.AspNetCore.Mvc.ModelBinding.Internal
+namespace Microsoft.AspNetCore.Mvc.Internal
 {
     // Used as a placeholder to break cycles while building a tree of model binders in ModelBinderFactory.
     //

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBinderFactory.cs

@@ -9,7 +9,7 @@ using System.Linq;
 using System.Runtime.CompilerServices;
 using Microsoft.AspNetCore.Mvc.Core;
 using Microsoft.AspNetCore.Mvc.Internal;
-using Microsoft.AspNetCore.Mvc.ModelBinding.Internal;
+using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
 using Microsoft.Extensions.Internal;
 using Microsoft.Extensions.Options;
 
@@ -23,7 +23,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
         private readonly IModelMetadataProvider _metadataProvider;
         private readonly IModelBinderProvider[] _providers;
 
-        private readonly ConcurrentDictionary<object, IModelBinder> _cache;
+        private readonly ConcurrentDictionary<Key, IModelBinder> _cache;
 
         /// <summary>
         /// Creates a new <see cref="ModelBinderFactory"/>.
@@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
             _metadataProvider = metadataProvider;
             _providers = options.Value.ModelBinderProviders.ToArray();
 
-            _cache = new ConcurrentDictionary<object, IModelBinder>(ReferenceEqualityComparer.Instance);
+            _cache = new ConcurrentDictionary<Key, IModelBinder>();
         }
 
         /// <inheritdoc />
@@ -46,31 +46,52 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 throw new ArgumentNullException(nameof(context));
             }
 
-            // We perform caching in CreateBinder (not in CreateBinderCore) because we only want to
-            // cache the top-level binder.
             IModelBinder binder;
-            if (context.CacheToken != null && _cache.TryGetValue(context.CacheToken, out binder))
+            if (TryGetCachedBinder(context.Metadata, context.CacheToken, out binder))
             {
                 return binder;
             }
-
+            
+            // Perf: We're calling the Uncached version of the API here so we can:
+            // 1. avoid allocating a context when the value is already cached
+            // 2. avoid checking the cache twice when the value is not cached
             var providerContext = new DefaultModelBinderProviderContext(this, context);
-            binder = CreateBinderCore(providerContext, context.CacheToken);
+            binder = CreateBinderCoreUncached(providerContext, context.CacheToken);
             if (binder == null)
             {
                 var message = Resources.FormatCouldNotCreateIModelBinder(providerContext.Metadata.ModelType);
                 throw new InvalidOperationException(message);
             }
 
-            if (context.CacheToken != null)
+            Debug.Assert(!(binder is PlaceholderBinder));
+            AddToCache(context.Metadata, context.CacheToken, binder);
+
+            return binder;
+        }
+
+        // Called by the DefaultModelBinderProviderContext when we're recursively creating a binder
+        // so that all intermediate results can be cached.
+        private IModelBinder CreateBinderCoreCached(DefaultModelBinderProviderContext providerContext, object token)
+        {
+            IModelBinder binder;
+            if (TryGetCachedBinder(providerContext.Metadata, token, out binder))
             {
-                _cache.TryAdd(context.CacheToken, binder);
+                return binder;
+            }
+
+            // We're definitely creating a binder for an non-root node here, so it's OK for binder creation
+            // to fail.
+            binder = CreateBinderCoreUncached(providerContext, token) ?? NoOpBinder.Instance;
+
+            if (!(binder is PlaceholderBinder))
+            {
+                AddToCache(providerContext.Metadata, token, binder);
             }
 
             return binder;
         }
 
-        private IModelBinder CreateBinderCore(DefaultModelBinderProviderContext providerContext, object token)
+        private IModelBinder CreateBinderCoreUncached(DefaultModelBinderProviderContext providerContext, object token)
         {
             if (!providerContext.Metadata.IsBindingAllowed)
             {
@@ -82,32 +103,34 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
             // happen looking at just model metadata.
             var key = new Key(providerContext.Metadata, token);
 
-            // If we're currently recursively building a binder for this type, just return
+            // The providerContext.Visited is used here to break cycles in recursion. We need a separate
-            // a PlaceholderBinder. We'll fix it up later to point to the 'real' binder
+            // per-operation cache for cycle breaking because the global cache (_cache) needs to always stay
-            // when the stack unwinds.
+            // in a valid state.
-            var stack = providerContext.Stack;
+            //
-            for (var i = 0; i < stack.Count; i++)
+            // We store null as a sentinel inside the providerContext.Visited to track the fact that we've visited
+            // a given node but haven't yet created a binder for it. We don't want to eagerly create a
+            // PlaceholderBinder because that would result in lots of unnecessary indirection and allocations.
+            var visited = providerContext.Visited;
+
+            IModelBinder binder;
+            if (visited.TryGetValue(key, out binder))
             {
-                var entry = stack[i];
+                if (binder != null)
-                if (key.Equals(entry.Key))
                 {
-                    if (entry.Value == null)
+                    return binder;
-                    {
-                        // Recursion detected, create a DelegatingBinder.
-                        var binder = new PlaceholderBinder();
-                        stack[i] = new KeyValuePair<Key, PlaceholderBinder>(entry.Key, binder);
-                        return binder;
-                    }
-                    else
-                    {
-                        return entry.Value;
-                    }
                 }
+
+                // If we're currently recursively building a binder for this type, just return
+                // a PlaceholderBinder. We'll fix it up later to point to the 'real' binder
+                // when the stack unwinds.
+                binder = new PlaceholderBinder();
+                visited[key] = binder;
+                return binder;
             }
 
-            // OK this isn't a recursive case (yet) so "push" an entry on the stack and then ask the providers
+            // OK this isn't a recursive case (yet) so add an entry and then ask the providers
             // to create the binder.
-            stack.Add(new KeyValuePair<Key, PlaceholderBinder>(key, null));
+            visited.Add(key, null);
 
             IModelBinder result = null;
 
@@ -121,26 +144,49 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 }
             }
 
-            if (result == null && stack.Count > 1)
+            // If the PlaceholderBinder was created, then it means we recursed. Hook it up to the 'real' binder.
+            var placeholderBinder = visited[key] as PlaceholderBinder;
+            if (placeholderBinder != null)
             {
-                // Use a no-op binder if we're below the top level. At the top level, we throw.
+                // It's also possible that user code called into `CreateBinder` but then returned null, we don't
-                result = NoOpBinder.Instance;
+                // want to create something that will null-ref later so just hook this up to the no-op binder.
+                placeholderBinder.Inner = result ?? NoOpBinder.Instance;
             }
 
-            // "pop"
+            if (result != null)
-            Debug.Assert(stack.Count > 0);
-            var delegatingBinder = stack[stack.Count - 1].Value;
-            stack.RemoveAt(stack.Count - 1);
-
-            // If the DelegatingBinder was created, then it means we recursed. Hook it up to the 'real' binder.
-            if (delegatingBinder != null)
             {
-                delegatingBinder.Inner = result;
+                visited[key] = result;
             }
 
             return result;
         }
 
+        private void AddToCache(ModelMetadata metadata, object cacheToken, IModelBinder binder)
+        {
+            Debug.Assert(metadata != null);
+            Debug.Assert(binder != null);
+
+            if (cacheToken == null)
+            {
+                return;
+            }
+
+            _cache.TryAdd(new Key(metadata, cacheToken), binder);
+        }
+
+        private bool TryGetCachedBinder(ModelMetadata metadata, object cacheToken, out IModelBinder binder)
+        {
+            Debug.Assert(metadata != null);
+
+            if (cacheToken == null)
+            {
+                binder = null;
+                return false;
+            }
+
+            return _cache.TryGetValue(new Key(metadata, cacheToken), out binder);
+        }
+
         private class DefaultModelBinderProviderContext : ModelBinderProviderContext
         {
             private readonly ModelBinderFactory _factory;
@@ -161,7 +207,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 };
 
                 MetadataProvider = _factory._metadataProvider;
-                Stack = new List<KeyValuePair<Key, PlaceholderBinder>>();
+                Visited = new Dictionary<Key, IModelBinder>();
             }
 
             private DefaultModelBinderProviderContext(
@@ -172,7 +218,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
 
                 _factory = parent._factory;
                 MetadataProvider = parent.MetadataProvider;
-                Stack = parent.Stack;
+                Visited = parent.Visited;
 
                 BindingInfo = new BindingInfo()
                 {
@@ -189,16 +235,32 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
 
             public override IModelMetadataProvider MetadataProvider { get; }
 
-            // Not using a 'real' Stack<> because we want random access to modify the entries.
+            public Dictionary<Key, IModelBinder> Visited { get; }
-            public List<KeyValuePair<Key, PlaceholderBinder>> Stack { get; }
 
             public override IModelBinder CreateBinder(ModelMetadata metadata)
             {
+                if (metadata == null)
+                {
+                    throw new ArgumentNullException(nameof(metadata));
+                }
+
+                // For non-root nodes we use the ModelMetadata as the cache token. This ensures that all non-root
+                // nodes with the same metadata will have the the same binder. This is OK because for an non-root
+                // node there's no opportunity to customize binding info like there is for a parameter.
+                var token = metadata;
+
                 var nestedContext = new DefaultModelBinderProviderContext(this, metadata);
-                return _factory.CreateBinderCore(nestedContext, token: null);
+                return _factory.CreateBinderCoreCached(nestedContext, token);
             }
         }
 
+        // This key allows you to specify a ModelMetadata which represents the type/property being bound
+        // and a 'token' which acts as an arbitrary discriminator.
+        //
+        // This is necessary because the same metadata might be bound as a top-level parameter (with BindingInfo on
+        // the ParameterDescriptor) or in a call to TryUpdateModel (no BindingInfo) or as a collection element.
+        //
+        // We need to be able to tell the difference between these things to avoid over-caching.
         private struct Key : IEquatable<Key>
         {
             private readonly ModelMetadata _metadata;
@@ -228,6 +290,18 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 hash.Add(RuntimeHelpers.GetHashCode(_token));
                 return hash;
             }
+
+            public override string ToString()
+            {
+                if (_metadata.MetadataKind == ModelMetadataKind.Type)
+                {
+                    return $"{_token} (Type: '{_metadata.ModelType.Name}')";
+                }
+                else
+                {
+                    return $"{_token} (Property: '{_metadata.ContainerType.Name}.{_metadata.PropertyName}' Type: '{_metadata.ModelType.Name}')";
+                }
+            }
         }
     }
 }

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBinderFactoryTest.cs

@@ -2,6 +2,7 @@
 // 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 Microsoft.AspNetCore.Mvc.Internal;
 using Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
 using Microsoft.AspNetCore.Mvc.ModelBinding.Internal;
@@ -342,6 +343,244 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
             Assert.Same(modelBinder, result);
         }
 
+        [Fact]
+        public void CreateBinder_Caches_NonRootNodes()
+        {
+            // Arrange
+            var metadataProvider = new TestModelMetadataProvider();
+
+            var options = new TestOptionsManager<MvcOptions>();
+
+            IModelBinder inner = null;
+
+            var widgetProvider = new TestModelBinderProvider(c =>
+            {
+                if (c.Metadata.ModelType == typeof(Widget))
+                {
+                    var binder = c.CreateBinder(c.Metadata.Properties[nameof(Widget.Id)]);
+                    if (inner == null)
+                    {
+                        inner = binder;
+                    }
+                    else
+                    {
+                        Assert.Same(inner, binder);
+                    }
+
+                    return Mock.Of<IModelBinder>();
+                }
+
+                return null;
+            });
+
+            var widgetIdProvider = new TestModelBinderProvider(c =>
+            {
+                Assert.Equal(typeof(WidgetId), c.Metadata.ModelType);
+                return Mock.Of<IModelBinder>();
+            });
+
+            options.Value.ModelBinderProviders.Add(widgetProvider);
+            options.Value.ModelBinderProviders.Add(widgetIdProvider);
+
+            var factory = new ModelBinderFactory(metadataProvider, options);
+
+            var context = new ModelBinderFactoryContext()
+            {
+                Metadata = metadataProvider.GetMetadataForType(typeof(Widget)),
+                CacheToken = null, // We want the outermost provider to run twice.
+            };
+
+            // Act
+            var result1 = factory.CreateBinder(context);
+            var result2 = factory.CreateBinder(context);
+
+            // Assert
+            Assert.NotSame(result1, result2);
+
+            Assert.Equal(2, widgetProvider.SuccessCount);
+            Assert.Equal(1, widgetIdProvider.SuccessCount);
+        }
+
+        [Fact]
+        public void CreateBinder_Caches_NonRootNodes_WhenNonRootNodeReturnsNull()
+        {
+            // Arrange
+            var metadataProvider = new TestModelMetadataProvider();
+
+            var options = new TestOptionsManager<MvcOptions>();
+
+            IModelBinder inner = null;
+
+            var widgetProvider = new TestModelBinderProvider(c =>
+            {
+                if (c.Metadata.ModelType == typeof(Widget))
+                {
+                    var binder = c.CreateBinder(c.Metadata.Properties[nameof(Widget.Id)]);
+                    Assert.IsType<NoOpBinder>(binder);
+                    if (inner == null)
+                    {
+                        inner = binder;
+                    }
+                    else
+                    {
+                        Assert.Same(inner, binder);
+                    }
+
+                    return Mock.Of<IModelBinder>();
+                }
+
+                return null;
+            });
+
+            var widgetIdProvider = new TestModelBinderProvider(c =>
+            {
+                Assert.Equal(typeof(WidgetId), c.Metadata.ModelType);
+                return null;
+            });
+
+            options.Value.ModelBinderProviders.Add(widgetProvider);
+            options.Value.ModelBinderProviders.Add(widgetIdProvider);
+
+            var factory = new ModelBinderFactory(metadataProvider, options);
+
+            var context = new ModelBinderFactoryContext()
+            {
+                Metadata = metadataProvider.GetMetadataForType(typeof(Widget)),
+                CacheToken = null, // We want the outermost provider to run twice.
+            };
+
+            // Act
+            var result1 = factory.CreateBinder(context);
+            var result2 = factory.CreateBinder(context);
+
+            // Assert
+            Assert.NotSame(result1, result2);
+
+            Assert.Equal(2, widgetProvider.SuccessCount);
+            Assert.Equal(0, widgetIdProvider.SuccessCount);
+        }
+
+        // The fact that we use the ModelMetadata as the token is important for caching
+        // and sharing with TryUpdateModel.
+        [Fact]
+        public void CreateBinder_Caches_NonRootNodes_UsesModelMetadataAsToken()
+        {
+            // Arrange
+            var metadataProvider = new TestModelMetadataProvider();
+
+            var options = new TestOptionsManager<MvcOptions>();
+
+            IModelBinder inner = null;
+
+            var widgetProvider = new TestModelBinderProvider(c =>
+            {
+                if (c.Metadata.ModelType == typeof(Widget))
+                {
+                    inner = c.CreateBinder(c.Metadata.Properties[nameof(Widget.Id)]);
+                    return Mock.Of<IModelBinder>();
+                }
+
+                return null;
+            });
+
+            var widgetIdProvider = new TestModelBinderProvider(c =>
+            {
+                Assert.Equal(typeof(WidgetId), c.Metadata.ModelType);
+                return Mock.Of<IModelBinder>();
+            });
+
+            options.Value.ModelBinderProviders.Add(widgetProvider);
+            options.Value.ModelBinderProviders.Add(widgetIdProvider);
+
+            var factory = new ModelBinderFactory(metadataProvider, options);
+
+            var context = new ModelBinderFactoryContext()
+            {
+                Metadata = metadataProvider.GetMetadataForType(typeof(Widget)),
+                CacheToken = null,
+            };
+
+            // Act 1
+            var result1 = factory.CreateBinder(context);
+
+            context.Metadata = context.Metadata.Properties[nameof(Widget.Id)];
+            context.CacheToken = context.Metadata;
+
+            // Act 2
+            var result2 = factory.CreateBinder(context);
+
+            // Assert
+            Assert.Same(inner, result2);
+            Assert.Equal(1, widgetProvider.SuccessCount);
+            Assert.Equal(1, widgetIdProvider.SuccessCount);
+        }
+
+        // This is a really wierd case, but I wanted to make sure it's covered so it doesn't
+        // blow up in wierd ways.
+        //
+        // If a binder provider tries to recursively create itself, but then returns null, we've
+        // already returned and possibly cached the PlaceholderBinder instance, we want to make sure that
+        // instance won't nullref.
+        [Fact]
+        public void CreateBinder_Caches_NonRootNodes_FixesUpPlaceholderBinder()
+        {
+            // Arrange
+            var metadataProvider = new TestModelMetadataProvider();
+
+            var options = new TestOptionsManager<MvcOptions>();
+
+            IModelBinder inner = null;
+            IModelBinder innerInner = null;
+
+            var widgetProvider = new TestModelBinderProvider(c =>
+            {
+                if (c.Metadata.ModelType == typeof(Widget))
+                {
+                    inner = c.CreateBinder(c.Metadata.Properties[nameof(Widget.Id)]);
+                    return Mock.Of<IModelBinder>();
+                }
+
+                return null;
+            });
+
+            var widgetIdProvider = new TestModelBinderProvider(c =>
+            {
+                Assert.Equal(typeof(WidgetId), c.Metadata.ModelType);
+                innerInner = c.CreateBinder(c.Metadata);
+                return null;
+            });
+
+            options.Value.ModelBinderProviders.Add(widgetProvider);
+            options.Value.ModelBinderProviders.Add(widgetIdProvider);
+
+            var factory = new ModelBinderFactory(metadataProvider, options);
+
+            var context = new ModelBinderFactoryContext()
+            {
+                Metadata = metadataProvider.GetMetadataForType(typeof(Widget)),
+                CacheToken = null,
+            };
+
+            // Act 1
+            var result1 = factory.CreateBinder(context);
+
+            context.Metadata = context.Metadata.Properties[nameof(Widget.Id)];
+            context.CacheToken = context.Metadata;
+
+            // Act 2
+            var result2 = factory.CreateBinder(context);
+
+            // Assert
+            Assert.Same(inner, result2);
+            Assert.NotSame(inner, innerInner);
+
+            var placeholder = Assert.IsType<PlaceholderBinder>(innerInner);
+            Assert.IsType<NoOpBinder>(placeholder.Inner);
+
+            Assert.Equal(1, widgetProvider.SuccessCount);
+            Assert.Equal(0, widgetIdProvider.SuccessCount);
+        }
+
         private class Widget
         {
             public WidgetId Id { get; set; }
@@ -365,9 +604,17 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 _factory = factory;
             }
 
+            public int SuccessCount { get; private set; }
+
             public IModelBinder GetBinder(ModelBinderProviderContext context)
             {
-                return _factory(context);
+                var binder = _factory(context);
+                if (binder != null)
+                {
+                    SuccessCount++;
+                }
+
+                return binder;
             }
         }
     }