[release/9.0-staging] Fix to #35239 - EF9: SaveChanges() is significantly slower in .NET9 vs. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping (#35360)

* Fix to #35239 - EF9: SaveChanges() is significantly slower in .NET9 vs. .NET8 when using .ToJson() Mapping vs. PostgreSQL Legacy POCO mapping

Fixes #35239

Description

EF9 introduced a change in how we construct ValueComparers for some of our types (specifically collection of scalars/references), in preparation for AOT work. The way the change was implemented may cause a severe performance regression during SaveChanges operation involving multiple entities using collections of primitives (one of our highly requested features).

Customer impact

Customers performing data manipulation operations on entities with collections of primitives may experience significant performance regressions. This may also happen when no data has been changed, but sufficiently large entity graph has been loaded into change tracker. There is no workaround for this issue, apart from changing the model to not use primitive collections (which is unacceptable for majority of customers)

How found

Multiple customer reports on EF 9

Regression

Yes, from EF8. Note: this is a perf regression only, not a functional regression.

Testing

Ad hoc performance test using BenchmarkDotNet. Functional testing already covered by existing tests.

Risk

Low. The patch fix has been limited in scope to reduce the risk. Changes should only affect models with primitive collections. Added quirks just to be sure.

Author: maumar

src/EFCore.Cosmos/ChangeTracking/Internal/StringDictionaryComparer.cs

@@ -14,13 +14,25 @@ namespace Microsoft.EntityFrameworkCore.Cosmos.ChangeTracking.Internal;
 /// </summary>
 public sealed class StringDictionaryComparer<TDictionary, TElement> : ValueComparer<object>, IInfrastructure<ValueComparer>
 {
+    private static readonly bool UseOldBehavior35239 =
+        AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35239", out var enabled35239) && enabled35239;
+
     private static readonly MethodInfo CompareMethod = typeof(StringDictionaryComparer<TDictionary, TElement>).GetMethod(
+        nameof(Compare), BindingFlags.Static | BindingFlags.NonPublic, [typeof(object), typeof(object), typeof(Func<TElement, TElement, bool>)])!;
+
+    private static readonly MethodInfo LegacyCompareMethod = typeof(StringDictionaryComparer<TDictionary, TElement>).GetMethod(
         nameof(Compare), BindingFlags.Static | BindingFlags.NonPublic, [typeof(object), typeof(object), typeof(ValueComparer)])!;
 
     private static readonly MethodInfo GetHashCodeMethod = typeof(StringDictionaryComparer<TDictionary, TElement>).GetMethod(
+        nameof(GetHashCode), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable), typeof(Func<TElement, int>)])!;
+
+    private static readonly MethodInfo LegacyGetHashCodeMethod = typeof(StringDictionaryComparer<TDictionary, TElement>).GetMethod(
         nameof(GetHashCode), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable), typeof(ValueComparer)])!;
 
     private static readonly MethodInfo SnapshotMethod = typeof(StringDictionaryComparer<TDictionary, TElement>).GetMethod(
+        nameof(Snapshot), BindingFlags.Static | BindingFlags.NonPublic, [typeof(object), typeof(Func<TElement, TElement>)])!;
+
+    private static readonly MethodInfo LegacySnapshotMethod = typeof(StringDictionaryComparer<TDictionary, TElement>).GetMethod(
         nameof(Snapshot), BindingFlags.Static | BindingFlags.NonPublic, [typeof(object), typeof(ValueComparer)])!;
 
     /// <summary>
@@ -52,14 +64,56 @@ ValueComparer IInfrastructure<ValueComparer>.Instance
         var prm1 = Expression.Parameter(typeof(object), "a");
         var prm2 = Expression.Parameter(typeof(object), "b");
 
+        if (UseOldBehavior35239)
+        {
+            // (a, b) => Compare(a, b, new Comparer(...))
+            return Expression.Lambda<Func<object?, object?, bool>>(
+                Expression.Call(
+                    LegacyCompareMethod,
+                    prm1,
+                    prm2,
+#pragma warning disable EF9100
+                    elementComparer.ConstructorExpression),
+#pragma warning restore EF9100
+                prm1,
+                prm2);
+        }
+
+        // we check the compatibility between element type we expect on the Equals methods
+        // vs what we actually get from the element comparer
+        // if the expected is assignable from actual we can just do simple call...
+        if (typeof(TElement).IsAssignableFrom(elementComparer.Type))
+        {
+            // (a, b) => Compare(a, b, elementComparer.Equals)
+            return Expression.Lambda<Func<object?, object?, bool>>(
+                Expression.Call(
+                    CompareMethod,
+                    prm1,
+                    prm2,
+                    elementComparer.EqualsExpression),
+                prm1,
+                prm2);
+        }
+
+        // ...otherwise we need to rewrite the actual lambda (as we can't change the expected signature)
+        // in that case we are rewriting the inner lambda parameters to TElement and cast to the element comparer
+        // type argument in the body, so that semantics of the element comparison func don't change
+        var newInnerPrm1 = Expression.Parameter(typeof(TElement), "a");
+        var newInnerPrm2 = Expression.Parameter(typeof(TElement), "b");
+
+        var newEqualsExpressionBody = elementComparer.ExtractEqualsBody(
+            Expression.Convert(newInnerPrm1, elementComparer.Type),
+            Expression.Convert(newInnerPrm2, elementComparer.Type));
+
         return Expression.Lambda<Func<object?, object?, bool>>(
             Expression.Call(
                 CompareMethod,
                 prm1,
                 prm2,
-#pragma warning disable EF9100
-                elementComparer.ConstructorExpression),
-#pragma warning restore EF9100
+                Expression.Lambda(
+                    newEqualsExpressionBody,
+                    newInnerPrm1,
+                    newInnerPrm2)),
             prm1,
             prm2);
     }
@@ -68,32 +122,144 @@ private static Expression<Func<object, int>> GetHashCodeLambda(ValueComparer ele
     {
         var prm = Expression.Parameter(typeof(object), "o");
 
+        if (UseOldBehavior35239)
+        {
+            // o => GetHashCode((IEnumerable)o, new Comparer(...))
+            return Expression.Lambda<Func<object, int>>(
+                Expression.Call(
+                    LegacyGetHashCodeMethod,
+                    Expression.Convert(
+                        prm,
+                        typeof(IEnumerable)),
+#pragma warning disable EF9100
+                    elementComparer.ConstructorExpression),
+#pragma warning restore EF9100
+                prm);
+        }
+
+        if (typeof(TElement).IsAssignableFrom(elementComparer.Type))
+        {
+            // o => GetHashCode((IEnumerable)o, elementComparer.GetHashCode)
+            return Expression.Lambda<Func<object, int>>(
+                Expression.Call(
+                    GetHashCodeMethod,
+                    Expression.Convert(
+                        prm,
+                        typeof(IEnumerable)),
+                        elementComparer.HashCodeExpression),
+                prm);
+        }
+
+        var newInnerPrm = Expression.Parameter(typeof(TElement), "o");
+
+        var newInnerBody = elementComparer.ExtractHashCodeBody(
+            Expression.Convert(
+                newInnerPrm,
+                elementComparer.Type));
+
         return Expression.Lambda<Func<object, int>>(
             Expression.Call(
                 GetHashCodeMethod,
                 Expression.Convert(
                     prm,
                     typeof(IEnumerable)),
-#pragma warning disable EF9100
-                elementComparer.ConstructorExpression),
-#pragma warning restore EF9100
+                Expression.Lambda(
+                    newInnerBody,
+                    newInnerPrm)),
             prm);
     }
 
     private static Expression<Func<object, object>> SnapshotLambda(ValueComparer elementComparer)
     {
         var prm = Expression.Parameter(typeof(object), "source");
 
+        if (UseOldBehavior35239)
+        {
+            // source => Snapshot(source, new Comparer(..))
+            return Expression.Lambda<Func<object, object>>(
+                Expression.Call(
+                    LegacySnapshotMethod,
+                    prm,
+#pragma warning disable EF9100
+                    elementComparer.ConstructorExpression),
+#pragma warning restore EF9100
+                prm);
+        }
+
+        // TElement is both argument and return type so the types need to be the same
+        if (typeof(TElement) == elementComparer.Type)
+        {
+            // source => Snapshot(source, elementComparer.Snapshot)
+            return Expression.Lambda<Func<object, object>>(
+                Expression.Call(
+                    SnapshotMethod,
+                    prm,
+                    elementComparer.SnapshotExpression),
+                prm);
+        }
+
+        var newInnerPrm = Expression.Parameter(typeof(TElement), "source");
+
+        var newInnerBody = elementComparer.ExtractSnapshotBody(
+            Expression.Convert(
+                newInnerPrm,
+                elementComparer.Type));
+
+        // note we need to also convert the result of inner lambda back to TElement
         return Expression.Lambda<Func<object, object>>(
             Expression.Call(
                 SnapshotMethod,
                 prm,
-#pragma warning disable EF9100
-                elementComparer.ConstructorExpression),
-#pragma warning restore EF9100
+                Expression.Lambda(
+                    Expression.Convert(
+                        newInnerBody,
+                        typeof(TElement)),
+                    newInnerPrm)),
             prm);
     }
 
+    private static bool Compare(object? a, object? b, Func<TElement?, TElement?, bool> elementCompare)
+    {
+        if (ReferenceEquals(a, b))
+        {
+            return true;
+        }
+
+        if (a is null)
+        {
+            return b is null;
+        }
+
+        if (b is null)
+        {
+            return false;
+        }
+
+        if (a is IReadOnlyDictionary<string, TElement?> aDictionary && b is IReadOnlyDictionary<string, TElement?> bDictionary)
+        {
+            if (aDictionary.Count != bDictionary.Count)
+            {
+                return false;
+            }
+
+            foreach (var pair in aDictionary)
+            {
+                if (!bDictionary.TryGetValue(pair.Key, out var bValue)
+                    || !elementCompare(pair.Value, bValue))
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
+        throw new InvalidOperationException(
+            CosmosStrings.BadDictionaryType(
+                (a is IDictionary<string, TElement?> ? b : a).GetType().ShortDisplayName(),
+                typeof(IDictionary<,>).MakeGenericType(typeof(string), typeof(TElement)).ShortDisplayName()));
+    }
+
     private static bool Compare(object? a, object? b, ValueComparer elementComparer)
     {
         if (ReferenceEquals(a, b))
@@ -136,6 +302,27 @@ private static bool Compare(object? a, object? b, ValueComparer elementComparer)
                 typeof(IDictionary<,>).MakeGenericType(typeof(string), elementComparer.Type).ShortDisplayName()));
     }
 
+    private static int GetHashCode(IEnumerable source, Func<TElement?, int> elementGetHashCode)
+    {
+        if (source is not IReadOnlyDictionary<string, TElement?> sourceDictionary)
+        {
+            throw new InvalidOperationException(
+                CosmosStrings.BadDictionaryType(
+                    source.GetType().ShortDisplayName(),
+                    typeof(IList<>).MakeGenericType(typeof(TElement)).ShortDisplayName()));
+        }
+
+        var hash = new HashCode();
+
+        foreach (var pair in sourceDictionary)
+        {
+            hash.Add(pair.Key);
+            hash.Add(pair.Value == null ? 0 : elementGetHashCode(pair.Value));
+        }
+
+        return hash.ToHashCode();
+    }
+
     private static int GetHashCode(IEnumerable source, ValueComparer elementComparer)
     {
         if (source is not IReadOnlyDictionary<string, TElement?> sourceDictionary)
@@ -157,6 +344,25 @@ private static int GetHashCode(IEnumerable source, ValueComparer elementComparer
         return hash.ToHashCode();
     }
 
+    private static IReadOnlyDictionary<string, TElement?> Snapshot(object source, Func<TElement?, TElement?> elementSnapshot)
+    {
+        if (source is not IReadOnlyDictionary<string, TElement?> sourceDictionary)
+        {
+            throw new InvalidOperationException(
+                CosmosStrings.BadDictionaryType(
+                    source.GetType().ShortDisplayName(),
+                    typeof(IDictionary<,>).MakeGenericType(typeof(string), typeof(TElement)).ShortDisplayName()));
+        }
+
+        var snapshot = new Dictionary<string, TElement?>();
+        foreach (var pair in sourceDictionary)
+        {
+            snapshot[pair.Key] = pair.Value == null ? default : (TElement?)elementSnapshot(pair.Value);
+        }
+
+        return snapshot;
+    }
+
     private static IReadOnlyDictionary<string, TElement?> Snapshot(object source, ValueComparer elementComparer)
     {
         if (source is not IReadOnlyDictionary<string, TElement?> sourceDictionary)