Fix Dictionary perf regression for non-string ref type keys

[stephentoub]

Several releases ago, some performance improvements were made to Dictionary that were particularly valuable for value type keys, enabling equality comparisons to be devirtualized and possibly inlined. For non-string reference type keys, however, this can end up being a regression, as every dictionary access then needs to access EqualityComparer<TKey>.Default, which can be more expensive with shared generics. The access is hoisted out of a loop, but at least one per call is still needed.

This fixes the regression by ensuring we always store a comparer in the dictionary's constructor if TKey is a reference type. The same fix is applied to HashSet<T>. This also means we can delete some now unreachable code.

Method	Toolchain	Mean	Ratio
TryGetValue	\main\corerun.exe	14.53 us	1.00
TryGetValue	\pr\corerun.exe	11.91 us	0.80

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Collections.Generic;
using System.Linq;

[MemoryDiagnoser(displayGenColumns: false)]
[HideColumns("Job", "Error", "StdDev")]
public partial class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private C[] _keys;
    private Dictionary<C, int> _d;

    [GlobalSetup]
    public void Setup()
    {
        _keys = Enumerable.Range(0, 1000).Select(i => new C { Value = i }).ToArray();
        _d = new Dictionary<C, int>(_keys.Select(c => KeyValuePair.Create(c, c.Value)));
    }

    [Benchmark]
    public int TryGetValue()
    {
        int count = 0;
        foreach (C c in _keys)
            if (_d.TryGetValue(c, out _))
                count++;
        return count;
    }
}

class C
{
    public int Value;
    public override int GetHashCode() => Value;
    public override bool Equals(object obj) => obj is C c && Value == c.Value;
}

[ghost]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Several releases ago, some performance improvements were made to Dictionary that were particularly valuable for value type keys, enabling equality comparisons to be devirtualized and possibly inlined. For non-string reference type keys, however, this can end up being a regression, as every dictionary access then needs to access EqualityComparer<TKey>.Default, which can be more expensive with shared generics. The access is hoisted out of a loop, but at least one per call is still needed.

This fixes the regression by ensuring we always store a comparer in the dictionary's constructor if TKey is a reference type. The same fix is applied to HashSet<T>. This also means we can delete some now unreachable code.

  Method |         Toolchain |     Mean | Ratio |

------------ |------------------ |---------:|------:|
TryGetValue | \main\corerun.exe | 14.53 us | 1.00 |
TryGetValue | \pr\corerun.exe | 11.91 us | 0.80 |

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Collections.Generic;
using System.Linq;

[MemoryDiagnoser(displayGenColumns: false)]
[HideColumns("Job", "Error", "StdDev")]
public partial class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    private C[] _keys;
    private Dictionary<C, int> _d;

    [GlobalSetup]
    public void Setup()
    {
        _keys = Enumerable.Range(0, 1000).Select(i => new C { Value = i }).ToArray();
        _d = new Dictionary<C, int>(_keys.Select(c => KeyValuePair.Create(c, c.Value)));
    }

    [Benchmark]
    public int TryGetValue()
    {
        int count = 0;
        foreach (C c in _keys)
            if (_d.TryGetValue(c, out _))
                count++;
        return count;
    }
}

class C
{
    public int Value;
    public override int GetHashCode() => Value;
    public override bool Equals(object obj) => obj is C c && Value == c.Value;
}

Author:	stephentoub
Assignees:	-
Labels:	area-System.Collections
Milestone:	-

[jkotas]

Do we have micro-benchmarks that cover the case that this is improving?

[stephentoub]

Do we have micro-benchmarks that cover the case that this is improving?

Not merged; we generally only cover int and string, e.g.
https://github.com/dotnet/performance/blob/b905d2cc8fed5e40c5e2789365aab5f7c8eb5d38/src/benchmarks/micro/libraries/System.Collections/TryGetValue/TryGetValueTrue.cs#L17-L18
I have local changes to augment it but have been waiting to put up a PR until this is done.