Replace HashSet with a Hybrid HashSet/List

to improve performance on less nested models

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/ValidationStack.cs

@@ -0,0 +1,73 @@
+// 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;
+using System.Diagnostics;
+using Microsoft.AspNetCore.Mvc.Internal;
+
+namespace Microsoft.AspNetCore.Mvc.ModelBinding.Internal
+{
+    public class ValidationStack
+    {
+        public int Count => HashSet?.Count ?? List.Count;
+
+        // We tested the performance of a list at size 15 and found it still better than hashset, but to avoid a costly
+        // O(n) search at larger n we set the cutoff to 20. If someone finds the point where they intersect feel free to change this number.
+        internal const int CutOff = 20;
+
+        internal List<object> List { get; } = new List<object>();
+
+        internal HashSet<object> HashSet { get; set; }
+
+        public bool Push(object model)
+        {
+            if (HashSet != null)
+            {
+                return HashSet.Add(model);
+            }
+
+            if (ListContains(model))
+            {
+                return false;
+            }
+
+            List.Add(model);
+
+            if (HashSet == null && List.Count > CutOff)
+            {
+                HashSet = new HashSet<object>(List, ReferenceEqualityComparer.Instance);
+            }
+
+            return true;
+        }
+
+        public void Pop(object model)
+        {
+            if (HashSet != null)
+            {
+                HashSet.Remove(model);
+            }
+            else
+            {
+                if (model != null)
+                {
+                    Debug.Assert(ReferenceEquals(List[List.Count - 1], model));
+                    List.RemoveAt(List.Count - 1);
+                }
+            }
+        }
+
+        private bool ListContains(object model)
+        {
+            for (var i = 0; i < List.Count; i++)
+            {
+                if (ReferenceEquals(model, List[i]))
+                {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+    }
+}

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs

@@ -5,6 +5,7 @@ using System;
 using System.Collections.Generic;
 using System.Runtime.CompilerServices;
 using Microsoft.AspNetCore.Mvc.Internal;
+using Microsoft.AspNetCore.Mvc.ModelBinding.Internal;
 
 namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
 {
@@ -27,7 +28,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
         private ModelMetadata _metadata;
         private IValidationStrategy _strategy;
 
-        private HashSet<object> _currentPath;
+        private ValidationStack _currentPath;
 
         /// <summary>
         /// Creates a new <see cref="ValidationVisitor"/>.
@@ -67,7 +68,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             _validationState = validationState;
 
             _modelState = actionContext.ModelState;
-            _currentPath = new HashSet<object>(ReferenceEqualityComparer.Instance);
+            _currentPath = new ValidationStack();
         }
 
         /// <summary>
@@ -156,7 +157,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
         {
             RuntimeHelpers.EnsureSufficientExecutionStack();
 
-            if (model != null && !_currentPath.Add(model))
+            if (model != null && !_currentPath.Push(model))
             {
                 // This is a cycle, bail.
                 return true;
@@ -176,6 +177,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             {
                 // Use the key on the entry, because we might not have entries in model state.
                 SuppressValidation(entry.Key);
+                _currentPath.Pop(model);
                 return true;
             }
 
@@ -352,7 +354,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
                 _visitor._model = _model;
                 _visitor._strategy = _strategy;
 
-                _visitor._currentPath.Remove(_newModel);
+                _visitor._currentPath.Pop(_newModel);
             }
         }
     }

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs

@@ -557,6 +557,42 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             Assert.Empty(entry.Errors);
         }
 
+        [Fact]
+        [ReplaceCulture]
+        public void Validate_ComplexType_SecondLevelCyclesNotFollowed_Invalid()
+        {
+            // Arrange
+            var actionContext = new ActionContext();
+            var modelState = actionContext.ModelState;
+            var validationState = new ValidationStateDictionary();
+
+            var validator = CreateValidator();
+
+            var person = new Person() { Name = "Billy" };
+            person.Family = new Family { Members = new List<Person> { person } };
+
+            var model = (object)person;
+
+            modelState.SetModelValue("parameter.Name", "Billy", "Billy");
+            validationState.Add(model, new ValidationStateEntry() { Key = "parameter" });
+
+            // Act
+            validator.Validate(actionContext, validationState, "parameter", model);
+
+            // Assert
+            Assert.False(modelState.IsValid);
+            AssertKeysEqual(modelState, "parameter.Name", "parameter.Profession");
+
+            var entry = modelState["parameter.Name"];
+            Assert.Equal(ModelValidationState.Valid, entry.ValidationState);
+            Assert.Empty(entry.Errors);
+
+            entry = modelState["parameter.Profession"];
+            Assert.Equal(ModelValidationState.Invalid, entry.ValidationState);
+            var error = Assert.Single(entry.Errors);
+            Assert.Equal(error.ErrorMessage, ValidationAttributeUtil.GetRequiredErrorMessage("Profession"));
+        }
+
         [Fact]
         [ReplaceCulture]
         public void Validate_ComplexType_CyclesNotFollowed_Invalid()
@@ -1138,6 +1174,13 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             public string Profession { get; set; }
 
             public Person Friend { get; set; }
+
+            public Family Family { get; set; }
+        }
+
+        private class Family
+        {
+            public List<Person> Members { get; set; }
         }
 
         private class Person2

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Internal/ValidationStackTest.cs

@@ -0,0 +1,128 @@
+// 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;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Mvc.ModelBinding.Internal
+{
+    public class ValidationStackTest
+    {
+        [Theory]
+        [InlineData(0)]
+        [InlineData(5)]
+        [InlineData(ValidationStack.CutOff + 1)]
+        public void Push_ReturnsFalseIfValueAlreadyExists(int preloadCount)
+        {
+            // Arrange
+            var validationStack = new TestValidationStack();
+            var model = "This is a value";
+
+            PreLoad(preloadCount, validationStack);
+
+            // Act & Assert
+            Assert.True(validationStack.Push(model));
+            Assert.False(validationStack.Push(model));
+        }
+
+        [Theory]
+        [InlineData(0)]
+        [InlineData(5)]
+        [InlineData(ValidationStack.CutOff + 1)]
+        public void Pop_RemovesValueFromTheStack(int preloadCount)
+        {
+            // Arrange
+            var validationStack = new TestValidationStack();
+            var model = "This is a value";
+
+            PreLoad(preloadCount, validationStack);
+
+            // Act
+            validationStack.Push(model);
+            validationStack.Pop(model);
+
+            // Assert
+            Assert.False(validationStack.Contains(model));
+        }
+
+        [Theory]
+        [InlineData(0)]
+        [InlineData(5)]
+        [InlineData(ValidationStack.CutOff + 1)]
+        public void Pop_DoesNotThrowIfValueIsNull(int preloadCount)
+        {
+            // Arrange
+            var validationStack = new TestValidationStack();
+
+            PreLoad(preloadCount, validationStack);
+
+            // Act & Assert
+            // Poping null when it's not there must not throw
+            validationStack.Pop(null);
+        }
+
+        [Fact]
+        public void PushingMoreThanCutOffElements_SwitchesToHashSet()
+        {
+            // Arrange
+            var size = ValidationStack.CutOff + 1;
+
+            var validationStack = new TestValidationStack();
+            var models = new List<Model>();
+            for (var i = 0; i < size; i++)
+            {
+                models.Add(new Model { Position = i });
+            }
+
+            // Act & Assert
+            foreach (var model in models)
+            {
+                validationStack.Push(model);
+            }
+
+            Assert.Equal(size, validationStack.Count);
+
+            models.Reverse();
+            foreach (var model in models)
+            {
+                validationStack.Pop(model);
+            }
+
+            Assert.Equal(0, validationStack.Count);
+            Assert.True(validationStack.UsingHashSet());
+        }
+
+        private void PreLoad(int preloadCount, ValidationStack stack)
+        {
+            for (var i = 0; i < preloadCount; i++)
+            {
+                stack.Push(i);
+            }
+        }
+
+        private class Model
+        {
+            public int Position { get; set; }
+        }
+
+        private class TestValidationStack : ValidationStack
+        {
+            public bool Contains(object model)
+            {
+                if (HashSet != null)
+                {
+                    return HashSet.Contains(model);
+                }
+                else
+                {
+                    return List.Contains(model);
+                }
+            }
+
+            public bool UsingHashSet()
+            {
+                return HashSet != null;
+            }
+        }
+    }
+}