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.8k stars 3.2k forks source link

[Perf] consider not using Expression.Invoke in CreateGetValueExpression for value with converter when possible #35129

Open maumar opened 1 week ago

maumar commented 1 week ago

We made a change (needed for AOT) to how we compute CreateGetValueExpression for properties with converter. Before AOT we would just incorporate ConvertFromProviderExpression into the shaper, but that expression may contain constants we can't construct in the precompiled query. So instead, we find the converter from the model (IF we have the IProperty), get ConvertFromProviderExpression lambda and use Expression.Invoke to call it dynamically. This works fine, but is slow.

Instead, we could consider peeking into the ConvertFromProviderExpression in compile time, and if it doesn't contain any problematic constants, inline the expression to avoid Invoke (like we used to do in pre-AOT days), and only resort to that if we must.

roji commented 1 week ago

@maumar is it really only that extra invoke - as opposed to inlining - that's responsible for that big a a performance difference? That's a bit counter-intuitive to me (but I could be wrong).

Regardless, we should try (if at all possible) to not vary the behavior here on whether there's a problematic constant in the converter logic or not - this introduces a cliff where everything works fast until you introduce a captured variable and suddenly things are inexplicably slow (assuming they're indeed slow).

maumar commented 1 week ago

@roji not sure about Invoke in the CreateGetValueExpression, but the one in ValueComparer has a big perf impact, see https://github.com/dotnet/efcore/pull/35128 Here might be way different because we only call this once per element in the row, as opposed to many times over for the value comparer, when we do the bucketing of collection results. I will do proper benchmarking so see what the impact really is. Invoke is definitely slower than inlining, but I do see your point about keeping things consistent.