search

dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.79k stars 3.19k forks source link

ValueComparers, mappings and genericity #11072

Closed roji closed 6 years ago

roji commented 6 years ago

@ajcvickers, while implementing array comparison I've stumbled onto an issue. Although a generic version of ValueComparer exists, it is merely a utility that wraps the generic functions it's given with non-generic ones. This is an issue with arrays: the array comparer needs to invoke its element mapping's comparer on each element in the array, but this operation currently boxes, creating lots of garbage.

Fortunately, at this point most elements have a null ValueComparer, which is a case the array comparer has to check for anyway, and then drop down to IEquatable<T>.

Ideally there would be a RelationalTypeMapping<T>, whose ValueComparer would be a ValueComparer<T> as well (the current built-in mappings would be changed for this as well, i.e. IntTypeMapping would inherit from RelationalTypeMapping<int>). There could even conceivably be other future uses for this.

What are your thoughts?

roji commented 6 years ago

Thinking about this some more, I'm wondering about what happens when values are compared even without a value comparer. If Equals(object) is used, that's a boxing operation right there...

ajcvickers commented 6 years ago

@roji With regard to the boxing and perf in general, this is a known issue and is being tracked by #9422. In the PR I specifically did not address this (doesn't fit for 2.1) but created the expression based comparers to setup for a much faster, non-boxing implementation. I admit, the composing of comparers together--i.e. one comparer that uses comparers for its elements--is not something I thought about, but I don't think there is anything stopping that work efficiently.

With regard to having a RelationalTypeMapping<T> that has a generic ValueComparer, I did consider this, but existing type mappings (from 2.0 providers or elsewhere) may not have this anyway, so I opted to make it optional everywhere since the case of no value comparer needed to be handled anyway.

ajcvickers commented 6 years ago

Oh, also, an IntTypeMapping is not necessarily used to map an int, because it may have a value converter on it. It's actually more often used to map to an int. (I actually don't like at all the we have lots of subclasses for type mappings, but it was kind of rushed in in 2.0 as part of the literal generation and now we have it. Learning experience.) Anyway, it's unfortunate that the ValueComparer acts on the model CLR type, but the type mapping class is more to do with the provider CLR type. Typically a ValueComparer set by the application will never get on the type mapping, and type mappings created by the provider are expected to be consistent, but a type mapping with conversions and a comparer can be tricky.

roji commented 6 years ago

OK, I can see I'm a bit confused about all this..

Regarding the composing of comparers and avoiding boxing... Looking over my current implementation, it's not clear how the array's comparer could call the element's comparer with going passing through object. Have I misunderstood the way to this, in that I'm supposed to pass a full expression tree for the entire comparison code, rather than a thin expression tree wrapper over a Compare function? If I do pass an expression tree implementing the entire comparison, would that eliminate the boxing as my tree gets integrated into your larger tree? And finally, ValueComparer still compiles my tree into a Func<object, object, bool>, suggesting that it would be used somewhere and cause boxing?

Thanks for your patience...

ajcvickers commented 6 years ago

@roji I need to think some more about the composition case. In my mind I was imagining that the comparer for collection types (for example) would just call a function, possibly generic, that would do the comparision. I didn't think about building comparers for elements into the tree. I guess I was assuming that the elements in the collection were all simple types, but I can see that that might not be the case.

roji commented 6 years ago

@ajcvickers sure. I still don't understand how it's supposed to work even for simple types, as long as the collection itself is generic... In other words, I have a single NpgsqlArrayTypeMapping which is instantiated for all element types. At some point it's going to have to call the element's ValueComparer's API, but the only one exposed accepts objects...

ajcvickers commented 6 years ago

@roji I was assuming that all the elements were "simple types" that didn't need a ValueColmaper.

roji commented 6 years ago

Oh I see. It's definitely true in most cases (int, string etc.) so the more complex case can be deferred if this is too much to fix. The main reason for raising this now is the risk that it may be harder to fix the ValueComparer API later once it's released.

ajcvickers commented 6 years ago

@roji I mad some updates in this PR where I made two changes relevant here:

It will take expression tree work to create a complex, non-boxing comparer that composes comparers for arbitrary sub-types, but it should certainly be possible. Let me know if you think there should be helper methods for "common" cases, such as arrays of a single type?

roji commented 6 years ago

@ajcvickers I'll sync up with your changes soon and will let you know how it goes.