-
Notifications
You must be signed in to change notification settings - Fork 2.6k
DictionarySlim backport improvements: remove null check on insert #22599
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,6 @@ | |
| using System.Diagnostics; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.Serialization; | ||
| using System.Threading; | ||
|
|
||
| namespace System.Collections.Generic | ||
| { | ||
|
|
@@ -53,6 +52,7 @@ private struct Entry | |
| private IEqualityComparer<TKey> _comparer; | ||
| private KeyCollection _keys; | ||
| private ValueCollection _values; | ||
| private static readonly Entry[] InitialEntries = new Entry[1] { new Entry() { hashCode = -1 } }; | ||
|
|
||
| // constants for serialization | ||
| private const string VersionName = "Version"; // Do not rename (binary serialization) | ||
|
|
@@ -80,6 +80,15 @@ public Dictionary(int capacity, IEqualityComparer<TKey> comparer) | |
| // To start, move off default comparer for string which is randomised | ||
| _comparer = (IEqualityComparer<TKey>)NonRandomizedStringEqualityComparer.Default; | ||
| } | ||
|
|
||
| // Default initialization to avoid null check for every insert | ||
| // Reads will fail and first add cause a resize into a real array | ||
| if (_entries is null) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't it always be null?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If capacity passed is > 0 we call Initialize(capacity)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then wouldn't this be an else off of that? Or initialize these first and then do that check?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep |
||
| { | ||
| _buckets = HashHelpers.SizeOneIntArray; | ||
| _entries = InitialEntries; | ||
| _freeList = -1; | ||
| } | ||
| } | ||
|
|
||
| public Dictionary(IDictionary<TKey, TValue> dictionary) : this(dictionary, null) { } | ||
|
|
@@ -474,11 +483,6 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) | |
| ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key); | ||
| } | ||
|
|
||
| if (_buckets == null) | ||
| { | ||
| Initialize(0); | ||
| } | ||
|
|
||
| Entry[] entries = _entries; | ||
| IEqualityComparer<TKey> comparer = _comparer; | ||
|
|
||
|
|
@@ -624,7 +628,9 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) | |
| else | ||
| { | ||
| int count = _count; | ||
| if (count == entries.Length) | ||
|
|
||
| // _entries.Length == 1 is dummy entries | ||
| if (count == entries.Length || entries.Length == 1) | ||
| { | ||
| Resize(); | ||
| bucket = ref _buckets[hashCode % _buckets.Length]; | ||
|
|
@@ -968,7 +974,9 @@ public int EnsureCapacity(int capacity) | |
| { | ||
| if (capacity < 0) | ||
| ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity); | ||
| int currentCapacity = _entries == null ? 0 : _entries.Length; | ||
|
|
||
| // _entries.Length == 1 is dummy entries | ||
| int currentCapacity = _entries.Length == 1 ? 0 : _entries.Length; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it's removing a null check but adding in one or more _entries.Length == 1 checks?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea was remove check null for main method that adds so here we don't have null anymore but dummy len. BTW I agree with @jkotas and this improvement is not good for Dictionary, but as I said I published my thought/result
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #22599 (comment) do you agree?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. Thanks.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm exploring if some improvement done on DictionartSlim(corefxlab) can improve Dictionary. |
||
| if (currentCapacity >= capacity) | ||
| return currentCapacity; | ||
| _version++; | ||
|
|
@@ -1006,7 +1014,9 @@ public void TrimExcess(int capacity) | |
| int newSize = HashHelpers.GetPrime(capacity); | ||
|
|
||
| Entry[] oldEntries = _entries; | ||
| int currentCapacity = oldEntries == null ? 0 : oldEntries.Length; | ||
|
|
||
| // _entries.Length == 1 is dummy entries | ||
| int currentCapacity = _entries.Length == 1 ? 0 : oldEntries.Length; | ||
| if (newSize >= currentCapacity) | ||
| return; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s_initialEntries