Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Improve Dictionary<K,T> CQ - Take 2 #15411

Closed
wants to merge 4 commits into from
Closed

Improve Dictionary<K,T> CQ - Take 2 #15411

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b3b741
Ref locals in TryInsert
benaadams Dec 7, 2017
e24d574
Ref return FindEntry
benaadams Dec 7, 2017
747ad1c
Use EqualityComparer<TKey>.Default
benaadams Dec 7, 2017
e37f85d
null check to inlinable
benaadams Dec 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 114 additions & 61 deletions src/mscorlib/shared/System/Collections/Generic/Dictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ private struct Entry
private const string KeyValuePairsName = "KeyValuePairs"; // Do not rename (binary serialization)
private const string ComparerName = "Comparer"; // Do not rename (binary serialization)

private static Entry s_nullEntry;

public Dictionary() : this(0, null) { }

public Dictionary(int capacity) : this(capacity, null) { }
Expand All @@ -72,10 +74,11 @@ public Dictionary(int capacity, IEqualityComparer<TKey> comparer)
{
if (capacity < 0) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity);
if (capacity > 0) Initialize(capacity);
this.comparer = comparer ?? EqualityComparer<TKey>.Default;
this.comparer = comparer;
Copy link

@omariom omariom Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can be :

if (comparer != EqualityComparer<TKey>.Default)
{
    this.comparer = comparer;
}

So when comparer param happens to be EqualityComparer<TKey>.Default we don't lose the optimization


if (this.comparer == EqualityComparer<string>.Default)
if (typeof(TKey) == typeof(string) && (comparer == null || this.comparer == EqualityComparer<string>.Default))
{
// To start, move off default comparer for string which is randomised
this.comparer = (IEqualityComparer<TKey>)NonRandomizedStringEqualityComparer.Default;
}
}
Expand Down Expand Up @@ -139,13 +142,7 @@ protected Dictionary(SerializationInfo info, StreamingContext context)
HashHelpers.SerializationInfoTable.Add(this, info);
}

public IEqualityComparer<TKey> Comparer
{
get
{
return comparer;
}
}
public IEqualityComparer<TKey> Comparer => comparer ?? EqualityComparer<TKey>.Default;

public int Count
{
Expand Down Expand Up @@ -210,20 +207,37 @@ public TValue this[TKey key]
{
get
{
int i = FindEntry(key);
if (i >= 0) return entries[i].value;
ThrowHelper.ThrowKeyNotFoundException(key);
return default(TValue);
if (key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

ref Entry entry = ref FindEntry(key, out bool found);
if (!found)
{
ThrowHelper.ThrowKeyNotFoundException(key);
}
return entry.value;
}
set
{
if (key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

bool modified = TryInsert(key, value, InsertionBehavior.OverwriteExisting);
Debug.Assert(modified);
}
}

public void Add(TKey key, TValue value)
{
if (key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

bool modified = TryInsert(key, value, InsertionBehavior.ThrowOnExisting);
Debug.Assert(modified); // If there was an existing key and the Add failed, an exception will already have been thrown.
}
Expand All @@ -235,8 +249,13 @@ void ICollection<KeyValuePair<TKey, TValue>>.Add(KeyValuePair<TKey, TValue> keyV

bool ICollection<KeyValuePair<TKey, TValue>>.Contains(KeyValuePair<TKey, TValue> keyValuePair)
{
int i = FindEntry(keyValuePair.Key);
if (i >= 0 && EqualityComparer<TValue>.Default.Equals(entries[i].value, keyValuePair.Value))
if (keyValuePair.Key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

ref Entry entry = ref FindEntry(keyValuePair.Key, out bool found);
if (found && EqualityComparer<TValue>.Default.Equals(entry.value, keyValuePair.Value))
{
return true;
}
Expand All @@ -245,8 +264,13 @@ bool ICollection<KeyValuePair<TKey, TValue>>.Contains(KeyValuePair<TKey, TValue>

bool ICollection<KeyValuePair<TKey, TValue>>.Remove(KeyValuePair<TKey, TValue> keyValuePair)
{
int i = FindEntry(keyValuePair.Key);
if (i >= 0 && EqualityComparer<TValue>.Default.Equals(entries[i].value, keyValuePair.Value))
if (keyValuePair.Key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

ref Entry entry = ref FindEntry(keyValuePair.Key, out bool found);
if (found && EqualityComparer<TValue>.Default.Equals(entry.value, keyValuePair.Value))
{
Remove(keyValuePair.Key);
return true;
Expand All @@ -269,7 +293,13 @@ public void Clear()

public bool ContainsKey(TKey key)
{
return FindEntry(key) >= 0;
if (key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

FindEntry(key, out bool found);
return found;
}

public bool ContainsValue(TValue value)
Expand All @@ -283,10 +313,9 @@ public bool ContainsValue(TValue value)
}
else
{
EqualityComparer<TValue> c = EqualityComparer<TValue>.Default;
for (int i = 0; i < count; i++)
{
if (entries[i].hashCode >= 0 && c.Equals(entries[i].value, value)) return true;
if (entries[i].hashCode >= 0 && EqualityComparer<TValue>.Default.Equals(entries[i].value, value)) return true;
}
}
return false;
Expand Down Expand Up @@ -338,7 +367,7 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte
}

info.AddValue(VersionName, version);
info.AddValue(ComparerName, comparer, typeof(IEqualityComparer<TKey>));
info.AddValue(ComparerName, comparer ?? EqualityComparer<TKey>.Default, typeof(IEqualityComparer<TKey>));
info.AddValue(HashSizeName, buckets == null ? 0 : buckets.Length); // This is the length of the bucket array

if (buckets != null)
Expand All @@ -349,22 +378,36 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte
}
}

private int FindEntry(TKey key)
private ref Entry FindEntry(TKey key, out bool found)
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @mikendn - this change should be evaluated separately from the Comparer change (submitting it as separate PR would be best).

{
if (key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}
Debug.Assert(key != null);

found = true;
if (buckets != null)
{
int hashCode = comparer.GetHashCode(key) & 0x7FFFFFFF;
for (int i = buckets[hashCode % buckets.Length]; i >= 0; i = entries[i].next)
int hashCode = (comparer == null ? EqualityComparer<TKey>.Default.GetHashCode(key) : comparer.GetHashCode(key)) & 0x7FFFFFFF;
Copy link

Choose a reason for hiding this comment

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

You can call GetHashCode directly on the key as enums don't box anymore.

Copy link

@omariom omariom Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

And I would extract it to separate methods. Though it creates one more copy for complex keys but inlining of EqualityComparer<TKey>.Default methods already introduces a couple of copies.

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private int GetKeyHashCode(TKey key)
        {
            if (default(TKey) != null)
            {
                return comparer != null ? comparer.GetHashCode(key) : key.GetHashCode();
            }
            else
            {
                return comparer.GetHashCode(key);
            }
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private bool KeysEqual(TKey x, TKey y)
        {
            if (default(TKey) != null)
            {
                return comparer != null ? comparer.Equals(x, y) : EqualityComparer<TKey>.Default.Equals(x, y);
            }
            else
            {
                return comparer.Equals(x, y);
            }
        }

Copy link
Member

Choose a reason for hiding this comment

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

Be careful about the class path ... AggressiveInlining is not always respected for it because of the runtime does not know how to inline the complex generic dictionary access that it introduces.

Copy link

Choose a reason for hiding this comment

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

Hmm.. it works for intas the key but I haven't yet checked tuples, complex structs and reference types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Naive branch prediction (non-loop) prefers continue than jump forward; so using default comparer in the first path (being more common than custom comparer) would be better.

Will change hashcode.

Copy link

Choose a reason for hiding this comment

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

That exactly why I changed the order :)

       ; comparer != null
       488B4E18             mov      rcx, gword ptr [rsi+24]
       4885C9               test     rcx, rcx
       750C                 jne      SHORT G_M33318_IG06
       ; inlined Defaul.Equals start
       3BD7                 cmp      edx, edi
       410F94C7             sete     r15b
       450FB6FF             movzx    r15, r15b 
       ; inlined Defaul.Equals end
       EB15                 jmp      SHORT G_M33318_IG07

G_M33318_IG06:
       448BC7               mov      r8d, edi
       49BB0001C36DFE070000 mov      r11, 0x7FE6DC30100
       3909                 cmp      dword ptr [rcx], ecx
       41FF13               call     qword ptr [r11]System.Collections.Generic.IEqualityComparer`1[Int32][System.Int32]:Equals(int,int):bool:this
       448BF8               mov      r15d, eax

G_M33318_IG07:
       4585FF               test     r15d, r15d
       751A                 jne      SHORT G_M33318_IG11

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh my days, you are right, the compilation reverses the tests 😢

Copy link

Choose a reason for hiding this comment

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

And yes, it doesn't inline for Dictionary2[__Canon, Int32]` 😢

call     System.Collections.Generic.Dictionary`2[__Canon,Int32][System.__Canon,System.Int32]:KeysEqual(ref,ref):bool:this

int targetBucket = hashCode % buckets.Length;
int i = buckets[targetBucket];
while (i >= 0)
{
if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key)) return i;
ref Entry candidateEntry = ref entries[i];
if (candidateEntry.hashCode == hashCode && ((comparer == null && EqualityComparer<TKey>.Default.Equals(candidateEntry.key, key)) || (comparer != null && comparer.Equals(candidateEntry.key, key))))
Copy link

Choose a reason for hiding this comment

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

Currently it checks comparer twice:

       test     rcx, rcx
       jne      SHORT G_M38470_IG06
       ;...
       test     rcx, rcx
       je       SHORT G_M38470_IG07

If you simplify it to

&& (comparer != null ? comparer.Equals(candidateEntry.key, key) : EqualityComparer<TKey>.Default.Equals(candidateEntry.key, key)

it will remove the second check.

{
return ref candidateEntry;
}

i = candidateEntry.next;
}
}
return -1;

found = false;
return ref NotFound;
}

private ref Entry NotFound
{
[MethodImpl(MethodImplOptions.NoInlining)]
get => ref s_nullEntry;
}

private void Initialize(int capacity)
Expand All @@ -378,23 +421,22 @@ private void Initialize(int capacity)

private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
{
if (key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}
Debug.Assert(key != null);

if (buckets == null) Initialize(0);
int hashCode = comparer.GetHashCode(key) & 0x7FFFFFFF;
int hashCode = (comparer == null ? EqualityComparer<TKey>.Default.GetHashCode(key) : comparer.GetHashCode(key)) & 0x7FFFFFFF;
int targetBucket = hashCode % buckets.Length;
int collisionCount = 0;

for (int i = buckets[targetBucket]; i >= 0; i = entries[i].next)
int i = buckets[targetBucket];
while (i >= 0)
{
if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key))
ref Entry candidateEntry = ref entries[i];
if (candidateEntry.hashCode == hashCode && ((comparer == null && EqualityComparer<TKey>.Default.Equals(candidateEntry.key, key)) || (comparer != null && comparer.Equals(candidateEntry.key, key))))
{
if (behavior == InsertionBehavior.OverwriteExisting)
{
entries[i].value = value;
candidateEntry.value = value;
version++;
return true;
}
Expand All @@ -406,6 +448,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)

return false;
}

i = candidateEntry.next;
collisionCount++;
}

Expand All @@ -427,19 +471,21 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
count++;
}

entries[index].hashCode = hashCode;
entries[index].next = buckets[targetBucket];
entries[index].key = key;
entries[index].value = value;
ref Entry entry = ref entries[index];
entry.hashCode = hashCode;
entry.next = buckets[targetBucket];
entry.key = key;
entry.value = value;
buckets[targetBucket] = index;
version++;

// If we hit the collision threshold we'll need to switch to the comparer which is using randomized string hashing
// i.e. EqualityComparer<string>.Default.

if (collisionCount > HashHelpers.HashCollisionThreshold && comparer is NonRandomizedStringEqualityComparer)
if (typeof(TKey) == typeof(string) && collisionCount > HashHelpers.HashCollisionThreshold && comparer is NonRandomizedStringEqualityComparer)
Copy link
Member

Choose a reason for hiding this comment

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

I do not think typeof(TKey) == typeof(string) check is going to help anything here.

Copy link

@redknightlois redknightlois Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@jkotas If I am reading that properly you are killing the whole branch when TKey is not an string and struct. You are also adding a branching check when TKey is a class. Could be a win if and only if the check introduced doesnt make its performance worse on the class path.

Copy link
Member

Choose a reason for hiding this comment

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

Right. Maybe the check can stay - but it should be after collisionCount > HashHelpers.HashCollisionThreshold to not regress the class path.

Copy link

Choose a reason for hiding this comment

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

Correct on that front.

Copy link
Member Author

Choose a reason for hiding this comment

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

Change change it to a struct check?

Copy link

Choose a reason for hiding this comment

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

How would you do that in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

if (default(TKey) == null && collisionCount > HashHelpers.HashCollisionThreshold ...

Copy link

Choose a reason for hiding this comment

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

Nice trick :D

{
comparer = (IEqualityComparer<TKey>)EqualityComparer<string>.Default;
// Clear comparer back to default, for randomised hashing
comparer = null;
Resize(entries.Length, true);
}

Expand Down Expand Up @@ -514,7 +560,7 @@ private void Resize(int newSize, bool forceNewHashCodes)
{
if (newEntries[i].hashCode != -1)
{
newEntries[i].hashCode = (comparer.GetHashCode(newEntries[i].key) & 0x7FFFFFFF);
newEntries[i].hashCode = (comparer == null ? EqualityComparer<TKey>.Default.GetHashCode(newEntries[i].key) : comparer.GetHashCode(newEntries[i].key)) & 0x7FFFFFFF;
}
}
}
Expand Down Expand Up @@ -545,15 +591,14 @@ public bool Remove(TKey key)

if (buckets != null)
{
int hashCode = comparer.GetHashCode(key) & 0x7FFFFFFF;
int hashCode = (comparer == null ? EqualityComparer<TKey>.Default.GetHashCode(key) : comparer.GetHashCode(key)) & 0x7FFFFFFF;
int bucket = hashCode % buckets.Length;
int last = -1;
int i = buckets[bucket];
while (i >= 0)
{
ref Entry entry = ref entries[i];

if (entry.hashCode == hashCode && comparer.Equals(entry.key, key))
if (entry.hashCode == hashCode && ((comparer == null && EqualityComparer<TKey>.Default.Equals(entry.key, key)) || (comparer != null && comparer.Equals(entry.key, key))))
{
if (last < 0)
{
Expand Down Expand Up @@ -599,15 +644,14 @@ public bool Remove(TKey key, out TValue value)

if (buckets != null)
{
int hashCode = comparer.GetHashCode(key) & 0x7FFFFFFF;
int hashCode = (comparer == null ? EqualityComparer<TKey>.Default.GetHashCode(key) : comparer.GetHashCode(key)) & 0x7FFFFFFF;
int bucket = hashCode % buckets.Length;
int last = -1;
int i = buckets[bucket];
while (i >= 0)
{
ref Entry entry = ref entries[i];

if (entry.hashCode == hashCode && comparer.Equals(entry.key, key))
if (entry.hashCode == hashCode && ((comparer == null && EqualityComparer<TKey>.Default.Equals(entry.key, key)) || (comparer != null && comparer.Equals(entry.key, key))))
{
if (last < 0)
{
Expand Down Expand Up @@ -647,17 +691,24 @@ public bool Remove(TKey key, out TValue value)

public bool TryGetValue(TKey key, out TValue value)
{
int i = FindEntry(key);
if (i >= 0)
if (key == null)
{
value = entries[i].value;
return true;
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}
value = default(TValue);
return false;

value = FindEntry(key, out bool found).value;
return found;
}

public bool TryAdd(TKey key, TValue value) => TryInsert(key, value, InsertionBehavior.None);
public bool TryAdd(TKey key, TValue value)
{
if (key == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

return TryInsert(key, value, InsertionBehavior.None);
}

bool ICollection<KeyValuePair<TKey, TValue>>.IsReadOnly
{
Expand Down Expand Up @@ -788,11 +839,13 @@ object IDictionary.this[object key]
{
if (IsCompatibleKey(key))
{
int i = FindEntry((TKey)key);
if (i >= 0)
if (key == null)
{
return entries[i].value;
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
}

ref var entry = ref FindEntry((TKey)key, out bool found);
return found ? (object)entry.value : null;
}
return null;
}
Expand Down