Allow custom equality comparer on ConcurrentDictionary TryUpdate

[skarllot]

The method TryUpdate from ConcurrentDictionary should allow to set a custom equality comparer for the value.

Rationale and Usage

A custom equality comparer for TryUpdate can target three problems.

Ahead Of Time

On some platforms is demanded static compilation ahead of time. On those platforms code generation is not allowed.
When the method TryUpdate use EqualityComparer<T>.Default for value type comparison, it attempts to use Reflection to instantiate a new type which implements the IEqualityComparer<T> interface. That operation is not allowed on AOT compiled code.

Performance

Currently the method TryUpdate uses the equality comparer instance from EqualityComparer<T>.Default which not be the most optimized comparison for the case.

Custom comparison

For external types not controlled by the user that does not implement the intended comparison it is needed to create a wrapper to overcome the lack of a proper comparison.
The same is true if a qualified comparison is needed for specific TryUpdate call.

Usage

Given those types:

readonly struct Person : IEquatable<Person>
{
    public long Id { get; }
    public string Name { get; }
    public string Email { get; }
    public int Version { get; }

    public Person(int version)
    {
        Id = default;
        Name = default;
        Email = default;
        Version = version;
    }

    public bool Equals(Person other) => Id == other.Id && Name == other.Name && Email == other.Email && Version == other.Version;
    public override bool Equals(object obj) => obj is Person other && Equals(other);
    public override int GetHashCode() => Id.GetHashCode();
}

sealed class VersioningComparer : IEqualityComparer<Person>
{
    public bool Equals(Person first, Person second) => first.Version == second.Version;
    public int GetHashCode(Person obj) => obj.Version;
}

Can be used as:

private readonly ConcurrentDictionary<long, Person> personDictionary;
private readonly VersioningComparer versionComparer;
// ...
public bool UpdatePerson(Person value)
{
    Person previousVersion = new Person(value.Version - 1);
    return personDictionary.TryUpdate(value.Id, value, previousVersion, versionComparer);
}

Proposed API

public class ConcurrentDictionary<TKey, TValue>
{
// ...
    public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue);
    public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue, System.Collections.Generic.IEqualityComparer<TValue> comparer);
// ...
}

Pull Request

A PR with the proposed changes is available: dotnet/corefx#28142

[skarllot]

I've rebased my PR and ran the tests

  xUnit.net console test runner (64-bit .NET Core)
  Copyright (C) 2014 Outercurve Foundation.

  Discovering: System.Collections.Concurrent.Tests
  Discovered:  System.Collections.Concurrent.Tests
  Starting:    System.Collections.Concurrent.Tests
  Finished:    System.Collections.Concurrent.Tests

  === TEST EXECUTION SUMMARY ===
     System.Collections.Concurrent.Tests  Total: 2079, Errors: 0, Failed: 0, Skipped: 0, Time: 16,444s
  ----- end 13:53:48,81 ----- exit code 0 ----------------------------------------------------------

[ghost]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

[skarllot]

I think that this issue is still relevant

[eiriktsarpalis]

Hi @skarllot, and apologies for the delayed response.

Doesn't passing the equality comparer at the constructor level address your use case?

I don't believe passing a custom equality comparer on a per-operation basis is practical or a good idea since:

• It would be impossible to look up an entry with the same key without making sure that the very same equality comparer is passed to the lookup operation.
• It would likely corrupt the underlying hashtable since it's possible for buckets to contain entries with invalid hashcodes under the default equality semantics for the instance.
• It's not clear how rehash operations would work for entries inserted using custom comparers.

In light of the above, I would not supporting adding such a feature to ConcurrentDictionary.

[skarllot]

Hello @eiriktsarpalis, the issue is that the TryUpdateInternal method uses default equality compare instead of the one passed to the constructor.

[eiriktsarpalis]

My apologies, I misunderstood your request as exposing a key equality comparer parameter. On quick inspection, it seems that the ConcurrentDictionary implementation is hardcoding EqualityComparer<TValue>.Default in quite a few places, so if we do parameterize it should be done on the constructor level.

You mention performance as a motivating factor for this request. Could you share any benchmarks showing performance improvements of your prototype? I should point out that since dotnet/coreclr#14125 the JIT will devirtualize EqualityComparer.Default.Equals calls so there might be an opportunity to slightly improve performance specifically in the TryUpdateInternal implementation.

cc @stephentoub

[skarllot]

I've mention performance because I could compare instances based on version number instead of all members values.

[eiriktsarpalis]

I suspect that this proposal might cover your use case.

[ghost]

This issue has been marked with the api-needs-work label. This may suggest that the proposal requires further refinement before it can be considered for API review. Please refer to our API review guidelines for a detailed description of the process.

When ready to submit an amended proposal, please ensure that the original post in this issue has been updated, following the API proposal template and examples as provided in the guidelines.