From 9a68f48a5cd79916f2529f139bac39ebeba8e7a5 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 13 Sep 2018 14:33:06 -0700 Subject: [PATCH] Minor improvments to RVD perf --- .../RouteValueDictionaryBenchmark.cs | 75 ++++-- .../RouteValueDictionary.cs | 160 +++++++++---- .../RouteValueDictionaryTests.cs | 215 +++++++++++++++++- 3 files changed, 391 insertions(+), 59 deletions(-) diff --git a/benchmarks/Microsoft.AspNetCore.Routing.Performance/RouteValueDictionaryBenchmark.cs b/benchmarks/Microsoft.AspNetCore.Routing.Performance/RouteValueDictionaryBenchmark.cs index 656a742367..522d8d3ed4 100644 --- a/benchmarks/Microsoft.AspNetCore.Routing.Performance/RouteValueDictionaryBenchmark.cs +++ b/benchmarks/Microsoft.AspNetCore.Routing.Performance/RouteValueDictionaryBenchmark.cs @@ -8,11 +8,11 @@ namespace Microsoft.AspNetCore.Routing { public class RouteValueDictionaryBenchmark { - // These dictionaries are used by a few tests over and over, so don't modify them destructively. private RouteValueDictionary _arrayValues; private RouteValueDictionary _propertyValues; - [GlobalSetup] + // We modify the route value dictionaries in many of these benchmarks. + [IterationSetup] public void Setup() { _arrayValues = new RouteValueDictionary() @@ -27,18 +27,57 @@ namespace Microsoft.AspNetCore.Routing [Benchmark] public RouteValueDictionary AddSingleItem() { - var dictionary = new RouteValueDictionary(); - dictionary.Add("action", "Index"); + var dictionary = new RouteValueDictionary + { + { "action", "Index" } + }; return dictionary; } [Benchmark] public RouteValueDictionary AddThreeItems() { - var dictionary = new RouteValueDictionary(); - dictionary.Add("action", "Index"); - dictionary.Add("controller", "Home"); - dictionary.Add("id", "15"); + var dictionary = new RouteValueDictionary + { + { "action", "Index" }, + { "controller", "Home" }, + { "id", "15" } + }; + return dictionary; + } + + [Benchmark] + public RouteValueDictionary ConditionalAdd_ContainsKeyAdd() + { + var dictionary = _arrayValues; + + if (!dictionary.ContainsKey("action")) + { + dictionary.Add("action", "Index"); + } + + if (!dictionary.ContainsKey("controller")) + { + dictionary.Add("controller", "Home"); + } + + if (!dictionary.ContainsKey("area")) + { + dictionary.Add("area", "Admin"); + } + + return dictionary; + } + + [Benchmark] + public RouteValueDictionary ConditionalAdd_TryAdd() + { + var dictionary = _arrayValues; + + dictionary.TryAdd("action", "Index"); + dictionary.TryAdd("controller", "Home"); + dictionary.TryAdd("area", "Admin"); + return dictionary; } @@ -56,7 +95,7 @@ namespace Microsoft.AspNetCore.Routing [Benchmark] public RouteValueDictionary ForEachThreeItems_Properties() { - var dictionary = _arrayValues; + var dictionary = _propertyValues; foreach (var kvp in dictionary) { GC.KeepAlive(kvp.Value); @@ -87,8 +126,10 @@ namespace Microsoft.AspNetCore.Routing [Benchmark] public RouteValueDictionary SetSingleItem() { - var dictionary = new RouteValueDictionary(); - dictionary["action"] = "Index"; + var dictionary = new RouteValueDictionary + { + ["action"] = "Index" + }; return dictionary; } @@ -103,10 +144,12 @@ namespace Microsoft.AspNetCore.Routing [Benchmark] public RouteValueDictionary SetThreeItems() { - var dictionary = new RouteValueDictionary(); - dictionary["action"] = "Index"; - dictionary["controller"] = "Home"; - dictionary["id"] = "15"; + var dictionary = new RouteValueDictionary + { + ["action"] = "Index", + ["controller"] = "Home", + ["id"] = "15" + }; return dictionary; } @@ -136,4 +179,4 @@ namespace Microsoft.AspNetCore.Routing return dictionary; } } -} +} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Routing.Abstractions/RouteValueDictionary.cs b/src/Microsoft.AspNetCore.Routing.Abstractions/RouteValueDictionary.cs index 2d30f6f25f..837e466d28 100644 --- a/src/Microsoft.AspNetCore.Routing.Abstractions/RouteValueDictionary.cs +++ b/src/Microsoft.AspNetCore.Routing.Abstractions/RouteValueDictionary.cs @@ -155,9 +155,9 @@ namespace Microsoft.AspNetCore.Routing { get { - if (string.IsNullOrEmpty(key)) + if (key == null) { - throw new ArgumentNullException(nameof(key)); + ThrowArgumentNullExceptionForKey(); } object value; @@ -167,9 +167,9 @@ namespace Microsoft.AspNetCore.Routing set { - if (string.IsNullOrEmpty(key)) + if (key == null) { - throw new ArgumentNullException(nameof(key)); + ThrowArgumentNullExceptionForKey(); } // We're calling this here for the side-effect of converting from properties @@ -177,7 +177,7 @@ namespace Microsoft.AspNetCore.Routing // property storage is immutable. EnsureCapacity(_count); - var index = FindInArray(key); + var index = FindIndex(key); if (index < 0) { EnsureCapacity(_count + 1); @@ -255,12 +255,12 @@ namespace Microsoft.AspNetCore.Routing { if (key == null) { - throw new ArgumentNullException(nameof(key)); + ThrowArgumentNullExceptionForKey(); } EnsureCapacity(_count + 1); - var index = FindInArray(key); + var index = FindIndex(key); if (index >= 0) { var message = Resources.FormatRouteValueDictionary_DuplicateKey(key, nameof(RouteValueDictionary)); @@ -302,7 +302,7 @@ namespace Microsoft.AspNetCore.Routing { if (key == null) { - throw new ArgumentNullException(nameof(key)); + ThrowArgumentNullExceptionForKey(); } return TryGetValue(key, out var _); @@ -362,7 +362,7 @@ namespace Microsoft.AspNetCore.Routing EnsureCapacity(Count); - var index = FindInArray(item.Key); + var index = FindIndex(item.Key); var array = _arrayStorage; if (index >= 0 && EqualityComparer.Default.Equals(array[index].Value, item.Value)) { @@ -380,7 +380,7 @@ namespace Microsoft.AspNetCore.Routing { if (key == null) { - throw new ArgumentNullException(nameof(key)); + ThrowArgumentNullExceptionForKey(); } if (Count == 0) @@ -390,7 +390,7 @@ namespace Microsoft.AspNetCore.Routing EnsureCapacity(Count); - var index = FindInArray(key); + var index = FindIndex(key); if (index >= 0) { _count--; @@ -404,14 +404,54 @@ namespace Microsoft.AspNetCore.Routing return false; } + /// + /// Attempts to the add the provided and to the dictionary. + /// + /// The key. + /// The value. + /// Returns true if the value was added. Returns false if the key was already present. + public bool TryAdd(string key, object value) + { + if (key == null) + { + ThrowArgumentNullExceptionForKey(); + } + + // Since this is an attempt to write to the dictionary, just make it an array if it isn't. If the code + // path we're on event tries to write to the dictionary, it will likely get 'upgraded' at some point, + // so we do it here to keep the code size and complexity down. + EnsureCapacity(Count); + + var index = FindIndex(key); + if (index >= 0) + { + return false; + } + + EnsureCapacity(Count + 1); + _arrayStorage[Count] = new KeyValuePair(key, value); + _count++; + return true; + } + /// public bool TryGetValue(string key, out object value) { if (key == null) { - throw new ArgumentNullException(nameof(key)); + ThrowArgumentNullExceptionForKey(); } + if (_propertyStorage == null) + { + return TryFindItem(key, out value); + } + + return TryGetValueSlow(key, out value); + } + + private bool TryGetValueSlow(string key, out object value) + { if (_propertyStorage != null) { var storage = _propertyStorage; @@ -423,25 +463,17 @@ namespace Microsoft.AspNetCore.Routing return true; } } - - value = default; - return false; - } - - var array = _arrayStorage; - for (var i = 0; i < _count; i++) - { - if (string.Equals(array[i].Key, key, StringComparison.OrdinalIgnoreCase)) - { - value = array[i].Value; - return true; - } } value = default; return false; } + private static void ThrowArgumentNullExceptionForKey() + { + throw new ArgumentNullException("key"); + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void EnsureCapacity(int capacity) { @@ -456,7 +488,10 @@ namespace Microsoft.AspNetCore.Routing if (_propertyStorage != null) { var storage = _propertyStorage; - capacity = Math.Max(storage.Properties.Length, capacity); + + // If we're converting from properties, it's likely due to an 'add' to make sure we have at least + // the default amount of space. + capacity = Math.Max(DefaultCapacity, Math.Max(storage.Properties.Length, capacity)); var array = new KeyValuePair[capacity]; for (var i = 0; i < storage.Properties.Length; i++) @@ -484,10 +519,14 @@ namespace Microsoft.AspNetCore.Routing } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private int FindInArray(string key) + private int FindIndex(string key) { + // Generally the bounds checking here will be elided by the JIT because this will be called + // on the same code path as EnsureCapacity. var array = _arrayStorage; - for (var i = 0; i < _count; i++) + var count = _count; + + for (var i = 0; i < count; i++) { if (string.Equals(array[i].Key, key, StringComparison.OrdinalIgnoreCase)) { @@ -498,9 +537,32 @@ namespace Microsoft.AspNetCore.Routing return -1; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private bool TryFindItem(string key, out object value) + { + var array = _arrayStorage; + var count = _count; + + // Elide bounds check for indexing. + if ((uint)count <= (uint)array.Length) + { + for (var i = 0; i < count; i++) + { + if (string.Equals(array[i].Key, key, StringComparison.OrdinalIgnoreCase)) + { + value = array[i].Value; + return true; + } + } + } + + value = null; + return false; + } + public struct Enumerator : IEnumerator> { - private RouteValueDictionary _dictionary; + private readonly RouteValueDictionary _dictionary; private int _index; public Enumerator(RouteValueDictionary dictionary) @@ -513,7 +575,7 @@ namespace Microsoft.AspNetCore.Routing _dictionary = dictionary; Current = default; - _index = -1; + _index = 0; } public KeyValuePair Current { get; private set; } @@ -524,22 +586,36 @@ namespace Microsoft.AspNetCore.Routing { } + // Similar to the design of List.Enumerator - Split into fast path and slow path for inlining friendliness + [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool MoveNext() { - if (++_index < _dictionary.Count) - { - if (_dictionary._propertyStorage != null) - { - var storage = _dictionary._propertyStorage; - var property = storage.Properties[_index]; - Current = new KeyValuePair(property.Name, property.GetValue(storage.Value)); - return true; - } + var dictionary = _dictionary; - Current = _dictionary._arrayStorage[_index]; + // The uncommon case is that the propertyStorage is in use + if (dictionary._propertyStorage == null && ((uint)_index < (uint)dictionary._count)) + { + Current = dictionary._arrayStorage[_index]; + _index++; return true; } + return MoveNextRare(); + } + + private bool MoveNextRare() + { + var dictionary = _dictionary; + if (dictionary._propertyStorage != null && ((uint)_index < (uint)dictionary._count)) + { + var storage = dictionary._propertyStorage; + var property = storage.Properties[_index]; + Current = new KeyValuePair(property.Name, property.GetValue(storage.Value)); + _index++; + return true; + } + + _index = dictionary._count; Current = default; return false; } @@ -547,7 +623,7 @@ namespace Microsoft.AspNetCore.Routing public void Reset() { Current = default; - _index = -1; + _index = 0; } } @@ -595,4 +671,4 @@ namespace Microsoft.AspNetCore.Routing } } } -} +} \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteValueDictionaryTests.cs b/test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteValueDictionaryTests.cs index 5cb45fc14c..1d4ec848af 100644 --- a/test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteValueDictionaryTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteValueDictionaryTests.cs @@ -380,6 +380,19 @@ namespace Microsoft.AspNetCore.Routing.Tests Assert.False(result); } + [Fact] + public void IndexGet_EmptyStringIsAllowed() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + var value = dict[""]; + + // Assert + Assert.Null(value); + } + [Fact] public void IndexGet_EmptyStorage_ReturnsNull() { @@ -486,6 +499,19 @@ namespace Microsoft.AspNetCore.Routing.Tests Assert.IsType[]>(dict._arrayStorage); } + [Fact] + public void IndexSet_EmptyStringIsAllowed() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + dict[""] = "foo"; + + // Assert + Assert.Equal("foo", dict[""]); + } + [Fact] public void IndexSet_EmptyStorage_UpgradesToList() { @@ -747,6 +773,19 @@ namespace Microsoft.AspNetCore.Routing.Tests Assert.IsType[]>(dict._arrayStorage); } + [Fact] + public void Add_EmptyStringIsAllowed() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + dict.Add("", "foo"); + + // Assert + Assert.Equal("foo", dict[""]); + } + [Fact] public void Add_PropertyStorage() { @@ -762,6 +801,14 @@ namespace Microsoft.AspNetCore.Routing.Tests kvp => { Assert.Equal("age", kvp.Key); Assert.Equal(30, kvp.Value); }, kvp => { Assert.Equal("key", kvp.Key); Assert.Equal("value", kvp.Value); }); Assert.IsType[]>(dict._arrayStorage); + + // The upgrade from property -> array should make space for at least 4 entries + Assert.Collection( + dict._arrayStorage, + kvp => Assert.Equal(new KeyValuePair("age", 30), kvp), + kvp => Assert.Equal(new KeyValuePair("key", "value"), kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp)); } [Fact] @@ -994,6 +1041,19 @@ namespace Microsoft.AspNetCore.Routing.Tests Assert.False(result); } + [Fact] + public void ContainsKey_EmptyStringIsAllowed() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + var result = dict.ContainsKey(""); + + // Assert + Assert.False(result); + } + [Fact] public void ContainsKey_PropertyStorage_False() { @@ -1206,6 +1266,19 @@ namespace Microsoft.AspNetCore.Routing.Tests Assert.False(result); } + [Fact] + public void Remove_EmptyStringIsAllowed() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + var result = dict.Remove(""); + + // Assert + Assert.False(result); + } + [Fact] public void Remove_PropertyStorage_Empty() { @@ -1320,6 +1393,132 @@ namespace Microsoft.AspNetCore.Routing.Tests Assert.IsType[]>(dict._arrayStorage); } + [Fact] + public void TryAdd_EmptyStringIsAllowed() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + var result = dict.TryAdd("", "foo"); + + // Assert + Assert.True(result); + } + + // We always 'upgrade' if you are trying to write to the dictionary. + [Fact] + public void TryAdd_ConvertsPropertyStorage_ToArrayStorage() + { + // Arrange + var dict = new RouteValueDictionary(new { key = "value", }); + + // Act + var result = dict.TryAdd("key", "value"); + + // Assert + Assert.False(result); + Assert.Null(dict._propertyStorage); + Assert.Collection( + dict._arrayStorage, + kvp => Assert.Equal(new KeyValuePair("key", "value"), kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp)); + } + + [Fact] + public void TryAdd_EmptyStorage_CanAdd() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + var result = dict.TryAdd("key", "value"); + + // Assert + Assert.True(result); + Assert.Collection( + dict._arrayStorage, + kvp => Assert.Equal(new KeyValuePair("key", "value"), kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp)); + } + + [Fact] + public void TryAdd_ArrayStorage_CanAdd() + { + // Arrange + var dict = new RouteValueDictionary() + { + { "key0", "value0" }, + }; + + // Act + var result = dict.TryAdd("key1", "value1"); + + // Assert + Assert.True(result); + Assert.Collection( + dict._arrayStorage, + kvp => Assert.Equal(new KeyValuePair("key0", "value0"), kvp), + kvp => Assert.Equal(new KeyValuePair("key1", "value1"), kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp)); + } + + [Fact] + public void TryAdd_ArrayStorage_CanAddWithResize() + { + // Arrange + var dict = new RouteValueDictionary() + { + { "key0", "value0" }, + { "key1", "value1" }, + { "key2", "value2" }, + { "key3", "value3" }, + }; + + // Act + var result = dict.TryAdd("key4", "value4"); + + // Assert + Assert.True(result); + Assert.Collection( + dict._arrayStorage, + kvp => Assert.Equal(new KeyValuePair("key0", "value0"), kvp), + kvp => Assert.Equal(new KeyValuePair("key1", "value1"), kvp), + kvp => Assert.Equal(new KeyValuePair("key2", "value2"), kvp), + kvp => Assert.Equal(new KeyValuePair("key3", "value3"), kvp), + kvp => Assert.Equal(new KeyValuePair("key4", "value4"), kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp)); + } + + [Fact] + public void TryAdd_ArrayStorage_DoesNotAddWhenKeyIsPresent() + { + // Arrange + var dict = new RouteValueDictionary() + { + { "key0", "value0" }, + }; + + // Act + var result = dict.TryAdd("key0", "value1"); + + // Assert + Assert.False(result); + Assert.Collection( + dict._arrayStorage, + kvp => Assert.Equal(new KeyValuePair("key0", "value0"), kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp), + kvp => Assert.Equal(default, kvp)); + } + [Fact] public void TryGetValue_EmptyStorage() { @@ -1335,6 +1534,20 @@ namespace Microsoft.AspNetCore.Routing.Tests Assert.Null(value); } + [Fact] + public void TryGetValue_EmptyStringIsAllowed() + { + // Arrange + var dict = new RouteValueDictionary(); + + // Act + var result = dict.TryGetValue("", out var value); + + // Assert + Assert.False(result); + Assert.Null(value); + } + [Fact] public void TryGetValue_PropertyStorage_False() { @@ -1618,4 +1831,4 @@ namespace Microsoft.AspNetCore.Routing.Tests public string State { get; set; } } } -} +} \ No newline at end of file