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

[maumar]

Problem was that as part of AOT refactoring we changed way that we build comparers. Specifically, comparers of collections - ListOfValueTypesComparer, ListOfNullableValueTypesComparer and ListOfReferenceTypesComparer. Before those list comparer Compare, Hashcode and Snapshot methods would take as argument element comparer, which was responsible for comparing elements. We need to be able to express these in code for AOT but we are not able to generate constant of type ValueComparer (or ValueComparer) that was needed. As a solution, each comparer now stores expression describing how it can be constructed, so we use that instead (as we are perfectly capable to expressing that in code form). Problem is that now every time compare, snapshot or hashcode method is called for array type, we construct new ValueComparer for the element type. As a result in the reported case we would generate 1000s of comparers which all have to be compiled and that causes huge overhead.

Fix is to pass relevant func from the element comparer to the outer comparer. We only passed the element comparer object to the outer Compare/Hashcode/Snapshot function to call that relevant func. This way we avoid constructing redundant comparers.

Fixes #35239

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

[maumar]

note: this PR only addresses ListOfValueTypesComparer and ListOfNullableValueTypesComparer - the issue also exists for ListOfReferenceTypesComparer and in Cosmos, but those are more tricky to fix as the TElement of the parent comparer doesn't necessarily match the TElement of the element comparer, so it's not always safe to copy over lambdas from element to parent.
THe aim is to make this PR safe enough for potential servicing and as it fixes the reported case (List<Guid>)

[roji]

@maumar for reference, could you include the benchmark results before and after this change, as well as the benchmark code itself?

[roji]

This seems like a bit of a hack, introducing a fast and slow path for when the value comparer doesn't match the type of the collection.

I understand that this PR is meant for servicing, but have you explored aligning the element and collection types instead, i.e. making sure the type on the element comparer always matches the TElement of the collection comparer? That seems like it might not be too complicated, introducing a upcast Convert node to the collection's TElement into the element converter (or possibly wrapping the element comparer in e.g. UpcastingValueComparer that would do that).

[maumar]

My current thinking was to introduce different comparer for cases where the types don't match and see what we can do there. Will experiment with the upcasting/wrapping. From what I saw most (almost all?) of these are in the nested collection cases. There is also stuff like new object[] { 1, 2, 3 } so the legacy path shouldn't actually be executed in a realistic scenario as these are supported in the model (none of our tests call the slow methods). So for patch I think it should be ok to fall back to the legacy path (we need to keep it anyway for quirk).

But yeah I would like to have something better as a final fix in 10. We definitely need it when nested collection support is added. Will experiment with aligning type elements like you suggested.

[roji]

I'll just point out that we don't have any particular time pressure on this (9.0.1 has sailed anyway, not sure when the cut-off is for 9.0.2, but not any time soon I think). So we can take our time and do it right, and then see whether backporting that makes sense.

[AndriySvyryd]

My thinking was that we shouldn't support new object[] { 1, 2, 3 } in 10 at all as that doesn't sound like a particularly useful scenario, and could be a pit of failure when the collection is built dynamically and has the potential of failing at runtime when one of the element types doesn't match. Supporting it would also complicate nested collection implementation.
But I wouldn't want to introduce a breaking change in a patch, so I think it's ok to keep it slow in 9.

[roji]

new object[] { 1, 2, 3 } may look useless, but some other scenarios aren't which have the same problem; for example, mixing different concrete types in a polymorphic collection (e.g. spatial types) - see #35332 (comment). I don't think there's a useless query form we can remove here (new object[] { 1, 2, 3 } ) without also removing legitimate scenarios (e.g. new Geometry[] { new Point(...), new Polygon(...) }, which then gets used in the query with Contains over some column).

At the end of the day, the problem here is simply that the element comparer's type doesn't correspond to the collection comparer's; that seems like a problem that we can fix without too much trouble (and why not do that regardless of the trouble we have here - it seems cleaner), and without introducing any breaking changes.

Assuming we agree that aligning the element comparer's type the collection comparer's is the ideal solution here (no breaking change, all queries supported, no perf trouble), then I'd prefer we started with that as the solution, rather than experimenting with hacks and alternative code paths; maybe that fix is patch worthy - we won't know until we try. If it isn't, at that point we can of course do hacks for the 9.0 perf issue.

[AndriySvyryd]

I think that the current change is the lowest risk way of fixing the reported regression in 9.0.x.
@maumar will check, but I believe that the scenarios that don't fall into the "fast path" either don't work at all in 9.0.0 (not counting InMemory) or don't actually use this code.

Perhaps this should just be committed to release/9.0-staging and we can keep discussing the 10.0 implementation in #35332

[roji]

I think that the current change is the lowest risk way of fixing the reported regression in 9.0.x.

It might be, but I'm having a hard time understanding why we're not first looking at the correct/right fix (assuming we agree on what's correct/right here), and then evaluating whether it makes sense to backport that or not (and only then hacking around). We don't have a close deadilne coming up, so I'd at least want us to try before doing something which we agree isn't the right fix (again, assuming we agree).

Experience shows that when we introduce hacks like this, they very frequently get left in, with a backlog issue saying "look into this" that never gets handled. If we don't have any specific pressure to hack, why not try to just do it right?

[AndriySvyryd]

It might be, but I'm having a hard time understanding why we're not first looking at the correct/right fix (assuming we agree on what's correct/right here), and then evaluating whether it makes sense to backport that or not (and only then hacking around). We don't have a close deadilne coming up, so I'd at least want us to try before doing something which we agree isn't the right fix (again, assuming we agree).

@maumar can comment on this. He tried, but there have been hurdles, that even if we can overcome the resulting fix would be riskier as the implementation would be significantly different.

The holidays are approaching so, we are closer to an effective deadline then the calendar would suggest, so I think we shouldn't delay the servicing fix that's already good enough while we try to agree on a perfect and elegant solution.

[roji]

OK. The cut-off for 9.0.2 is January 13th - that's around 10 days after the holidays, it really seems that we have enough time to at least attempt a better solution here.

In any case, I've gone ahead and implemented what I'd consider the right solution here - a simple ConvertingValueComparer that can be composed over the element value comparer, simply applying Convert nodes and exposing it as the base type (object in this case). See #35354 for the prototype.

@AndriySvyryd note that this is very similar to what you did in #33887 with NullableValueComparer; in fact, the proposed ConvertingValueComparer may be able to replace NullableValueComparer altogether, handling both Nullable<T> and arbitrary inheritance scenarios generically (not sure).

I do think there's a version of this which could meet the bar for servicing - it really doesn't seem that big of a deal; but if you're both really against it, we can patch the hack instead. In any case, I don't see a need for the breaking change in #35332, if we can just make sure all the types align cleanly etc.

[maumar]

I've updated the pr with cleaner approach. We no longer need the legacy code path, instead we reason about compatibility between lambda signatures (expected in the list comparer method and actual that we get from element comparer). I'm ok with going with the converting comparer as well, which works across the board, is more elegant and terse solution for sure. Pros of the solution here is no new APIs and a bit simpler Equals/Hashcode/Snapshot code - we don't always need to convert, e.g. when case of list of arrays (List<int[]>), outer comparer expects Equals of signature Func<int[], int[], bool> but element selector is typed as ValueComparer<IEnumerable>. However in this case we can get away with just passing the Func<IE<int>, IE<int>, bool> into Func<int[], int[], bool>. Problem only appears for snapshot (can't fit Func<IE<int>, IE<int>> into Func<int[], int[]>) so we do the rewite only there. ended up incorporating @roji 's concept of ConvertingValueComparer, but took advantage of contravariance - only doing conditional conversion based on assignability of target comparer type from the source type..

[maumar]

perf numbers

with warmup, so that comparers are/should be compiled:
8.0 - 77ms
9.0 - 593ms
9.0.2 - 98ms

no warmup:
8.0 - 84ms
9.0 - 650 ms
9.0.2 - 132ms

Will convert it to proper BDN and post code and more accurate numbers. But the improvement is significant.

[maumar]

BDN numbers:

8.0.11

Method	Mean	Error	StdDev
SaveChangesTest	172.1 ms	1.78 ms	1.58 ms

9.0

Method	Mean	Error	StdDev
SaveChangesTest	5.487 s	0.0621 s	0.0551 s

9.0.2 (with fix)

Method	Mean	Error	StdDev
SaveChangesTest	179.8 ms	1.71 ms	1.43 ms

benchmark code:

// See https://aka.ms/new-console-template for more information
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Running;
using Microsoft.EntityFrameworkCore;

BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args, DefaultConfig.Instance);

public class SaveChangesBenchmark
{
    private MyContext _ctx = new MyContext();

    [GlobalSetup]
    public virtual async Task Initialize()
    {
        var ctx = new MyContext();
        await ctx.Database.EnsureDeletedAsync();
        await ctx.Database.EnsureCreatedAsync();

        var seedData = ReturnSeedData();
        ctx.SampleEntities.AddRange(seedData);
        ctx.SaveChanges();
    }

    [IterationSetup]
    public void Setup()
    {
        _ctx = new MyContext();
        _ctx.ChangeTracker.Clear();

        foreach (var testId in Enumerable.Range(1, 200))
        {
            var src = _ctx.SampleEntities.Where(a => a.TestId == testId).FirstOrDefault();
        }
    }

    [Benchmark]
    public async Task SaveChangesTest()
    {
        await _ctx.SaveChangesAsync();
    }

    private static List<SampleEntity> ReturnSeedData()
    {
        var list = new List<SampleEntity>();

        for (int i = 0; i < 200; i++)
        {
            var s = new SampleEntity
            {
                TestId = i,
                RockId = Guid.NewGuid(),
                SccId = Guid.NewGuid(),
                AnotherId = Guid.NewGuid(),
                IsPushed = false,
                SeId = Guid.NewGuid(),
                SampleId = Guid.NewGuid(),
                Jsons = []
            };

            for (int j = 0; j < 300; j++)
            {
                var studentGradingNeed = new SampleJson
                {
                    ComponentGroupId = Guid.NewGuid(),
                    ComponentId = Guid.NewGuid(),
                    MonthId = Guid.NewGuid(),
                    IsGroupLevel = false,
                    MeasureId = Guid.NewGuid(),
                    OrderedComponentTrackingId = Guid.NewGuid(),
                    Result = new ResultJson
                    {
                        CreatedBy = Guid.NewGuid(),
                        CreatedByName = "Test",
                        CreatedDate = DateTime.UtcNow,
                        LastModifiedBy = Guid.NewGuid(),
                        LastModifiedByName = "Test",
                        LastModifiedDate = DateTime.UtcNow,
                        MarkIdValue = Guid.NewGuid(),
                        NumericValue = 44,
                        CommentIds = new List<Guid> { Guid.NewGuid() },
                        TextValue = "FF"
                    }
                };

                s.Jsons.Add(studentGradingNeed);
            }

            list.Add(s);
        }

        return list;
    }
}

public class MyContext : DbContext
{
    public DbSet<SampleEntity> SampleEntities { get; set; } = null!;

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer(@"Server=(localdb)\mssqllocaldb;Database=ReproSaveChangesBDN;Trusted_Connection=True;MultipleActiveResultSets=true");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.Entity<SampleEntity>().OwnsMany(c => c.Jsons, d =>
        {
            d.ToJson();
            d.OwnsOne(e => e.Result);
        });
    }
}


public class SampleEntity
{
    public Guid Id { get; set; }
    public Guid SampleId { get; set; }
    public Guid AnotherId { get; set; }
    public int TestId { get; set; }
    public Guid RockId { get; set; }
    public Guid SccId { get; set; }
    public Guid SeId { get; set; }

    public List<SampleJson> Jsons { get; set; } = [];
    public bool IsPushed { get; set; }
}

public record SampleJson
{

    public Guid TrackingId { get; set; }
    public Guid ComponentGroupId { get; set; }
    public Guid ComponentId { get; set; }
    public Guid? OrderedComponentTrackingId { get; set; }
    public Guid? MonthId { get; set; }
    public Guid? MeasureId { get; set; }
    public bool IsGroupLevel { get; set; }
    public ResultJson? Result { get; set; }
}

public record ResultJson
{
    public string? TextValue { get; set; }
    public decimal? NumericValue { get; set; }
    public Guid? MarkIdValue { get; set; }
    public List<Guid> CommentIds { get; set; } = [];
    public Guid CreatedBy { get; set; }
    public string CreatedByName { get; set; } = null!;
    public DateTime CreatedDate { get; set; }
    public Guid LastModifiedBy { get; set; }
    public string LastModifiedByName { get; set; } = null!;
    public DateTime LastModifiedDate { get; set; }
}

public interface ISampleDbContext
{
    DbSet<SampleEntity> SampleEntities { get; set; }
    void SetConnectionString(string connectionString);
}

[maumar]

I removed the constraint as conversions happen both ways

in case of object[] { 1, 2, 3 } target is target is object and source is int
in case of nested lists (List<List> target is List and source is object (because element comparer is ListOfReferenceTypesComparer which is typed as ValueComparer