Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Dictionary perf regression for non-string ref type keys #75663

Merged
merged 7 commits into from
Sep 23, 2022
Merged

Fix Dictionary perf regression for non-string ref type keys #75663

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f5d5e3a
Fix Dictionary perf regression for non-string ref type keys
stephentoub Sep 15, 2022
9f402a5
Address PR feedback
stephentoub Sep 15, 2022
9851911
Fix one more existing TODO
stephentoub Sep 15, 2022
3a27103
Revert previous PR feedback changes
stephentoub Sep 16, 2022
b746387
Address PR feedback
stephentoub Sep 16, 2022
48d6ca0
Merge branch 'dotnet:main' into dictcomparer
stephentoub Sep 16, 2022
00ce044
Merge branch 'dotnet:main' into dictcomparer
stephentoub Sep 21, 2022
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
2 changes: 1 addition & 1 deletion ...ollections/tests/Generic/Dictionary/Dictionary.Generic.Tests.ConcurrentAccessDetection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public async Task DictionaryConcurrentAccessDetection_ReferenceTypeKey(Type comp
IEqualityComparer<DummyRefType> customComparer = null;

Dictionary<DummyRefType, DummyRefType> dic = comparerType == null ?
new Dictionary<DummyRefType, DummyRefType>() :
new Dictionary<DummyRefType, DummyRefType>((customComparer = EqualityComparer<DummyRefType>.Default)) :
new Dictionary<DummyRefType, DummyRefType>((customComparer = (IEqualityComparer<DummyRefType>)Activator.CreateInstance(comparerType)));

var keyValueSample = new DummyRefType() { Value = 1 };
Expand Down
279 changes: 87 additions & 192 deletions src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,30 @@ public Dictionary(int capacity, IEqualityComparer<TKey>? comparer)
Initialize(capacity);
}

if (comparer is not null && comparer != EqualityComparer<TKey>.Default) // first check for null to avoid forcing default comparer instantiation unnecessarily
// For reference types, we always want to store a comparer instance, either
// the one provided, or if one wasn't provided, the default (accessing
// EqualityComparer<TKey>.Default with shared generics on every dictionary
// access can add measurable overhead). For value types, if no comparer is
// provided, or if the default is provided, we'd prefer to use
// EqualityComparer<TKey>.Default.Equals on every use, enabling the JIT to
// devirtualize and possibly inline the operation.
if (!typeof(TKey).IsValueType)
{
_comparer = comparer ?? EqualityComparer<TKey>.Default;
}
else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily
comparer != EqualityComparer<TKey>.Default)
{
_comparer = comparer;
}

// Special-case EqualityComparer<string>.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase.
// We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the
// hash buckets become unbalanced.
if (typeof(TKey) == typeof(string))
if (typeof(TKey) == typeof(string) &&
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer<string> stringComparer)
{
IEqualityComparer<string>? stringComparer = NonRandomizedStringEqualityComparer.GetStringComparer(_comparer);
if (stringComparer is not null)
{
_comparer = (IEqualityComparer<TKey>?)stringComparer;
}
_comparer = (IEqualityComparer<TKey>)stringComparer;
}
}

Expand Down Expand Up @@ -161,7 +170,8 @@ public IEqualityComparer<TKey> Comparer
{
if (typeof(TKey) == typeof(string))
{
return (IEqualityComparer<TKey>)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer<string?>?)_comparer);
Debug.Assert(_comparer is not null, "The comparer should never be null for a reference type.");
return (IEqualityComparer<TKey>)IInternalStringEqualityComparer.GetUnderlyingEqualityComparer((IEqualityComparer<string?>)_comparer);
}
else
{
Expand Down Expand Up @@ -364,71 +374,38 @@ internal ref TValue FindValue(TKey key)
IEqualityComparer<TKey>? comparer = _comparer;
if (comparer == null)
{
Debug.Assert(typeof(TKey).IsValueType);

uint hashCode = (uint)key.GetHashCode();
int i = GetBucket(hashCode);
Entry[]? entries = _entries;
uint collisionCount = 0;
if (typeof(TKey).IsValueType)
{
// ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic

i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
do
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test in if to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
goto ReturnNotFound;
}

entry = ref entries[i];
if (entry.hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entry.key, key))
{
goto ReturnFound;
}

i = entry.next;

collisionCount++;
} while (collisionCount <= (uint)entries.Length);

// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
goto ConcurrentOperation;
}
else
// ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
do
{
// Object type: Shared Generic, EqualityComparer<TValue>.Default won't devirtualize
// https://github.com/dotnet/runtime/issues/10050
// So cache in a local rather than get EqualityComparer per loop iteration
EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.Default;

i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
do
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test in if to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test in if to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
goto ReturnNotFound;
}
goto ReturnNotFound;
}

entry = ref entries[i];
if (entry.hashCode == hashCode && defaultComparer.Equals(entry.key, key))
{
goto ReturnFound;
}
entry = ref entries[i];
if (entry.hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entry.key, key))
{
goto ReturnFound;
}

i = entry.next;
i = entry.next;

collisionCount++;
} while (collisionCount <= (uint)entries.Length);
collisionCount++;
} while (collisionCount <= (uint)entries.Length);

// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
goto ConcurrentOperation;
}
// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
goto ConcurrentOperation;
}
else
{
Expand Down Expand Up @@ -521,85 +498,42 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)

if (comparer == null)
{
if (typeof(TKey).IsValueType)
Debug.Assert(typeof(TKey).IsValueType);
stephentoub marked this conversation as resolved.
Show resolved Hide resolved

// ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
while (true)
{
// ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
while (true)
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
break;
}

if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
{
if (behavior == InsertionBehavior.OverwriteExisting)
{
entries[i].value = value;
return true;
}

if (behavior == InsertionBehavior.ThrowOnExisting)
{
ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key);
}

return false;
}

i = entries[i].next;

collisionCount++;
if (collisionCount > (uint)entries.Length)
{
// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();
}
break;
}
}
else
{
// Object type: Shared Generic, EqualityComparer<TValue>.Default won't devirtualize
// https://github.com/dotnet/runtime/issues/10050
// So cache in a local rather than get EqualityComparer per loop iteration
EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.Default;
while (true)

if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
if (behavior == InsertionBehavior.OverwriteExisting)
{
break;
entries[i].value = value;
return true;
}

if (entries[i].hashCode == hashCode && defaultComparer.Equals(entries[i].key, key))
if (behavior == InsertionBehavior.ThrowOnExisting)
{
if (behavior == InsertionBehavior.OverwriteExisting)
{
entries[i].value = value;
return true;
}

if (behavior == InsertionBehavior.ThrowOnExisting)
{
ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key);
}

return false;
ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key);
}

i = entries[i].next;
return false;
}

collisionCount++;
if (collisionCount > (uint)entries.Length)
{
// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();
}
i = entries[i].next;

collisionCount++;
if (collisionCount > (uint)entries.Length)
{
// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();
}
}
}
Expand Down Expand Up @@ -718,67 +652,33 @@ internal static class CollectionsMarshalHelper

if (comparer == null)
{
if (typeof(TKey).IsValueType)
Debug.Assert(typeof(TKey).IsValueType);

// ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
while (true)
{
// ValueType: Devirtualize with EqualityComparer<TValue>.Default intrinsic
while (true)
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
break;
}

if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
{
exists = true;

return ref entries[i].value!;
}

i = entries[i].next;

collisionCount++;
if (collisionCount > (uint)entries.Length)
{
// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();
}
break;
}
}
else
{
// Object type: Shared Generic, EqualityComparer<TValue>.Default won't devirtualize
// https://github.com/dotnet/runtime/issues/10050
// So cache in a local rather than get EqualityComparer per loop iteration
EqualityComparer<TKey> defaultComparer = EqualityComparer<TKey>.Default;
while (true)

if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
break;
}

if (entries[i].hashCode == hashCode && defaultComparer.Equals(entries[i].key, key))
{
exists = true;

return ref entries[i].value!;
}

i = entries[i].next;

collisionCount++;
if (collisionCount > (uint)entries.Length)
{
// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();
}
exists = true;

return ref entries[i].value!;
}

i = entries[i].next;

collisionCount++;
if (collisionCount > (uint)entries.Length)
{
// The chain of entries forms a loop; which means a concurrent update has happened.
// Break out of the loop and throw, rather than looping forever.
ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported();
}
}
}
Expand Down Expand Up @@ -938,11 +838,6 @@ private void Resize(int newSize, bool forceNewHashCodes)
entries[i].hashCode = (uint)_comparer.GetHashCode(entries[i].key);
}
}

if (ReferenceEquals(_comparer, EqualityComparer<TKey>.Default))
{
_comparer = null;
}
}

// Assign member variables after both arrays allocated to guard against corruption from OOM if second fails
Expand Down
Loading