-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
New methods for EqualityComparer<T> #6407
Comments
This would probably best be done as a CLR optimization, rather than adding new public APIs that make up for deficiencies in the runtime. I've read for example that the JVM is able to inline virtual methods, although I don't know how easy this would be to do for the CLR.
I thought that was fixed with dotnet/coreclr#4340.
We'd end up having to maintain 2 different implementations of
Not necessarily, since most of those calls are made as them typed as
When the C# 7 feature is finally introduced, I think most people will find it more natural to compare the items one at a time, e.g. (name, age) = GetPersonTuple();
if (name == "John" && age == 13)
{
// ...
} as opposed to if (GetPersonTuple.Equals(("John", 13)))
{
} If people are concerned with performance, they should just avoid calling
That sounds very flaky, and would add an additional untaken branch for the cases where the comparer passed in was not the default comparer. It would also require additional bookkeeping logic on the part of Dictionary. |
In C# 7 instead of writing: (name, age) = GetPersonTuple();
if (name == "John" && age == 13)
{
// ...
} I think the following code would be as/more natural: (name, age) = GetPersonTuple();
if ((name, age) == ("Jhon", 13))
{
// ...
} OR tuple = GetPersonTuple();
if (tuple == ("Jhon", 13))
{
// ...
} Since creating a Tuple is going to be more easy, I expect more of them to be created. I would even use the When/if Tuple patterns arrive in C#, people might write it as |
Yes, but I think this specific case is unlikely to be implemented soon.
Can it be because
They will serve different purposes: direct calls to
We will be here and won't allow it to happen )
An example of real code: protected bool Set<T>(ref T field, T value, [CallerMemberName] string propertyName = null)
{
if (string.IsNullOrEmpty(propertyName))
ThrowArgumentNullException(nameof(propertyName));
if (!EqualityComparer<T>.Default.Equals(field, value))
{
field = value;
RaisePropertyChanged(propertyName);
return true;
}
return false;
}
Just 2 aggressively inlined methods in Dictionary. Minimal changes for Imagine a |
Good idea! |
I understand the concerns raised by @jamesqo, but I have to say I think the wins here are just too good to ignore, and I am skeptical we can get similar wins by doing the work in the runtime[s] anytime soon. i.e. I would be nice we our optimized solved this problem, but I think these API is a more practical (viable from cost/value perspective) way to get similar wins. |
In the specific example you've shown, I think the overhead of raising the event/etc. and whatnot will be much greater than the overhead of a single virtual call.
It's not called at all in List. I recently submitted a PR in fact (#6212) to optimize List.Contains to use @omariom @KrzysztofCwalina Maybe we should instead add |
ValueTuple is in CoreFx. |
A different solution would be to make the JIT use the type information of values stored in a Here, the JIT could use the known receiver type to devirtualize the calls. If there are concerns regarding reflection setting readonly fields: a) The JIT already disregards that and b) it could generate This optimization would help in all kinds of situations. |
As @omariom mentioned before, it doesn't look like the JIT does any devirtualization at the moment. I think a more practical thing to do would be to recognize code patterns of |
The .NET Native compiler for UWP does similar transformation of This optimization would make RyuJIT better AOT codegenerator for CoreRT project, closer to the .NET Native compiler for UWP compiler. |
What have been decided? |
Even if we introduced a new API, we would still need to do special things for it in the runtime/JIT to make it work well - without performance cliffs. So this issue looks like runtime/JIT optimization to me. The new APIs maybe nice, but they are not required to realize the perf benefits. The beauty of doing this as JIT optimization is that it will kick in for all existing code, without any changes. |
Then I am closing this issue in favor of a JIT specific one. |
EqualityComparer<T>.Default
is convenient when we need the ability to compare values to be passed around as a thing. But if just need to callEquals(x,y)
in a generic context we have to pay the cost of not inlined dynamicaly dispatched calls plus checks that the static field holding the default comparer has been initialized. It is especially bad for primitive and simple structs.I propose adding the following static methods to
EqualityComparer<T>
:Types and code that could benefit from the change:
1. Any code that calls EqualityComparer.Default.Equals and EqualityComparer.Default.GetHashCode where
T
is a primitive type.2. Tuples
This is how a call to
ValueTuple<int, long>.Equals
looks now:and how it could look with direct calls to
DefaulsEquals
.3. Dictionaries
Currently each successful lookup requires at least 2 interface calls to
comparer
'sGetHashCode
andEquals
. We could check if the comparer Dictionary gets is null or the default one and devitalize the calls asDefaultGetHashCode
andDefaultEquals
.In my experiments it makes lookups 40-45% faster when
TKey
isint
.p.s.
Wishes
We could implement
DefaultGetHashCode
andDefaultEquals
as:but
typeof(T).IsValueType
,typeof(T).IEnum
are not JIT time constants yet andx is IEquatable<T>
is not treated by JIT as generic constraint./cc @stephentoub @jkotas @KrzysztofCwalina @jamesqo @mikedn
The text was updated successfully, but these errors were encountered: