Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Conversation

@drieseng
Copy link
Contributor

  • Reduce allocations when RouteValueDictionary is initialized from another empty (array-based) RouteValueDictionary.
  • When RouteValueDictionary is initialized from another non-empty array-based RouteValueDictionary, then only allocate an array that corresponds with the number of items in that RouteValueDictionary instead of the capacity.
  • Refactor ContainsKey(string key) to use newly introduced ContainsKeyArray(string key) and ContainsKeyProperties(string key) methods that do not obtain the value of the key. This mostly affects the path where we're using property storage.
  • Update Add(string key, object value) and TryAdd(string key, object value) to use ContainsKeyArray(string key) to check whether an entry with the specified key already exists.
  • Improve performance of TryAdd(string key, object value) when backed by properties, and eliminate extra array allocation when number of properties is equal to capacity.

Benchmarks:

Method Version Mean Error StdDev Op/s Gen 0 Allocated
ContainsKey_Properties_Found master 49.79 ns 0.2453 ns 0.1915 ns 20,085,908.7 - 0 B
ContainsKey_Properties_Found PR 39.03 ns 1.7731 ns 1.9708 ns 25,623,247.9 - 0 B
ContainsKey_Properties_NotFound master 35.19 ns 0.1114 ns 0.0663 ns 28,415,384.2 - 0 B
ContainsKey_Properties_NotFound PR 31.73 ns 0.0754 ns 0.0589 ns 31,514,773.8 - 0 B
TryAdd_Properties_AtCapacity_KeyExists master 218.22 ns 2.2617 ns 2.1156 ns 4,582,526.4 0.0024 208 B
TryAdd_Properties_AtCapacity_KeyExists PR 150.07 ns 1.8355 ns 1.7169 ns 6,663,347.2 0.0014 120 B
TryAdd_Properties_AtCapacity_KeyDoesNotExist master 151.90 ns 1.2008 ns 1.0645 ns 6,583,092.7 0.0012 120 B
TryAdd_Properties_AtCapacity_KeyDoesNotExist PR 144.29 ns 2.0227 ns 1.8920 ns 6,930,577.9 0.0014 120 B
TryAdd_Properties_NotAtCapacity_KeyExists master 200.75 ns 2.1600 ns 2.0204 ns 4,981,297.3 0.0021 200 B
TryAdd_Properties_NotAtCapacity_KeyExists PR 145.98 ns 2.4794 ns 2.3192 ns 6,850,315.0 0.0010 112 B

{
var storage = new PropertyStorage(values);
_propertyStorage = storage;
_count = storage.Properties.Length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t this case leave the _arrayStorage uninitialized now? All other cases have an explicit initialization but this case does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does indeed leave array storage uninitialized, but that's not an issue as either:

  • property storage is initialized and used
    -or-
  • array storage is initialiazed and used

When initialized, we always prefer property storage over array storage. Array storage can remain uninitialized when property storage is initialized.

When property storage is initialized, but we want to manipulate the values, then the property storage is transformed to array storage. This is done using RouteValueDictionary.EnsureCapacitySlow(int capacity).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah cool, I missed that every use of the array storage would ensure the capacity before which would cause an initialization of the storage.

Since the checks always check whether the property storage is set (instead of checking when the array storage is not set), does it really make such a difference to not initialize the array storage to an empty array by default? All those _arrayStorage = Array.Empty… lines really seem verbose when they are always executed except in the one case where an explicit array is being set, or when the property storage is being set which does not need the array storage to be not set though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought it would be clearer when either _propertyStorage or _arrayStorage would be initialized, and not both.

@Tratcher Tratcher added this to the 3.0.0 milestone Nov 14, 2018
@JamesNK JamesNK merged commit 5da68a2 into aspnet:master Nov 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants