From 60c59b576e69d90bdb9eb26d702c199dd9ab92c5 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Fri, 7 Oct 2016 10:50:38 -0700 Subject: [PATCH] Replace HashSet with a Hybrid HashSet/List to improve performance on less nested models --- .../ModelBinding/Internal/ValidationStack.cs | 73 ++++++++++ .../Validation/ValidationVisitor.cs | 10 +- .../Internal/DefaultObjectValidatorTests.cs | 43 ++++++ .../Internal/ValidationStackTest.cs | 128 ++++++++++++++++++ 4 files changed, 250 insertions(+), 4 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/ValidationStack.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Internal/ValidationStackTest.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/ValidationStack.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Internal/ValidationStack.cs new file mode 100644 index 0000000000..ba24828544 --- /dev/null +++ b/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 List { get; } = new List(); + + internal HashSet 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(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; + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index 4e6ba85f1d..7ad300021f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/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 _currentPath; + private ValidationStack _currentPath; /// /// Creates a new . @@ -67,7 +68,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation _validationState = validationState; _modelState = actionContext.ModelState; - _currentPath = new HashSet(ReferenceEqualityComparer.Instance); + _currentPath = new ValidationStack(); } /// @@ -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); } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs index 3151a241c2..fa65b2cc5c 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs +++ b/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 } }; + + 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 Members { get; set; } } private class Person2 diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Internal/ValidationStackTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Internal/ValidationStackTest.cs new file mode 100644 index 0000000000..f4698abd52 --- /dev/null +++ b/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(); + 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; + } + } + } +}