Capability to disable #14001 by itself

[Prinsn]

What appears to be caused by #14001

Moving from 2.2 to 3.1 has shown a performance degradation in a scaling benchmark of 24 seconds to >= 50 minutes (based on estimate, have not let it run to completion, but at 1000 of 5000 entities processed, one block of code has gone from 64 ms to 723 ms, and at that point in time, metrics estimate about 50 minutes assuming progressive performance degradation flattens.

The best option I could find, provided at https://entityframeworkcore.com/knowledge-base/61074160/dbset-tentity--local-any---performance-issue-when-upgraging-ef-core-from-2-2-6-to-3-1-3, is to basically wrap all relevant code in a disable/reenable block but this has implications with how code is written, such that a DbSet extension for dbSet.Local.FirstOrDefault(...) ?? dbSet.FirstOrDefault(...) has no reference to the context it belongs to (that I'm aware of) to make such a modification.

I believe that it was in err to make such a drastic change as it affects existing code. This is unlike the async ConfigureAwait that is an improvement in most cases, as the price of development is to just tack it onto all code forever more for a typical gain.

It seems to me the correct behavior is to offer two properties, one that disables this change to live next to the global disable, such that both need to be enabled, and it defaults to enabled, for it to happen. Being able to disable this change to retain code performance in an upgrade without having to do a dance or refactors.

Further, this should be listed as a breaking change in the documentation https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-3.0/breaking-changes as it is otherwise obfuscated. It took approximately 6 hours of investigation to isolate the cause enough to be able to begin searching for solutions. It was purely by process of elimination that I could point to DbSet.Local as being a candidate for the problem.

However, it has been provided by another member of my team that this extension method may otherwise provide the same solution, meaning that anyone needing to gain this feature can use

public static LocalView<T> LocalNoChangeCheck<T>(this DbSet<T> dbset)
    where T : class
{
    var dbContext = dbset.GetService<DbContext>();
    var origEnabled = dbContext.ChangeTracker.AutoDetectChangesEnabled;
 
    if (origEnabled)
    {
        dbContext.ChangeTracker.AutoDetectChangesEnabled = false;
    }
 
    try
    {
        return dbset.Local;
    }
    finally
    {
        dbContext.ChangeTracker.AutoDetectChangesEnabled = origEnabled;
    }
}

So it is possible that this is "easily mitigable" by the developer that it's not worth addressing.

However, I believe it'd be the correct thing to do, because that is a massive change in how one would expect EFCore to function, and how it likely has been developed around. I don't see how it's an improvement to anyone that wasn't already assuming that the code change by #14001 was already the case and just eating the performance that the fix addressed that can be paired with a 7500x performance degradation in otherwise functional code is not something that should come with an "upgrade" without it being plainly obvious why and how to fix it (and that that fix should be easy instead of potentially major refactors)

[ajcvickers]

@Prinsn In retrospect, I agree that we should have documented this as a breaking change since the potential for perf differences is massive. However, it was a bug that DetectChanges was not being called automatically in this case, and I do believe that the making the fix to this was the right thing do to help ensure the local collection has fresh tracking data. If your application is designed such that DetectChanges is not needed here, then disabling it is reasonable. Also, consider using notification entities to avoid the need for DetectChanges at all. Finally, the result of .Local can be cached and reused in many situations.

[Prinsn]

That's reasonable, though I never had an issue with working directly off of local in EF, though I've not worked with it in 2-3 years. We regularly made use of a standard load pattern to fetch from local else hit the DB and I have no recollection of it being a critical performance piece, unlike now. Caching them seems like an obvious solution, but naive in nature, as that appears to be duplication of the local context, where it appears having a "non detecting local" access method serves the same purpose without using additional memory or architecture.

…

On Thu, Jul 9, 2020, 2:54 PM Arthur Vickers ***@***.***> wrote: @Prinsn <https://github.com/Prinsn> In retrospect, I agree that we should have documented this as a breaking change since the potential for perf differences is massive. However, it was a bug that DetectChanges was not being called automatically in this case, and I do believe that the making the fix to this was the right thing do to help ensure the local collection has fresh tracking data. If your application is designed such that DetectChanges is not needed here, then disabling it is reasonable. Also, consider using notification entities <https://blog.oneunicorn.com/2016/11/16/notification-entities-in-ef-core-1-1/> to avoid the need for DetectChanges at all. Finally, the result of .Local can be cached and reused in many situations. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21564 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYR7G6JDCLRUPA2G5RFSO3R2YG6RANCNFSM4OU5CI7A> .

[fiseni]

@Prinsn
DbSet.Local in EF6 is calling DetectChanges, thus keeping data in sync. But, I think whenever you call Local, it returns ObservableCollection copy. That's why looping through Local is fast.
On the other hand EFCore 1&2 was not keeping data in sync at all. If you switched from EF6 to EFCore1&2, most probably you had spike in performance. On top of not detecting changes, was not converting to ObservableCollection either.

If you're utilizing some pattern of loading the data, and then all the time working with Local only, for me personally that would be a risky move in EFCore1&2. Let me give you an example

// Load the data
var orders = context.Orders.Include(x => x.OrderLines).Load();
var testOrder = orders.FirstOrDefault()
// Remove one entity from child collection
testOrder.OrderLines.Remove(testOrder.OrderLines.FirstOrDefault());

After the removal, now context.OrderLines.Local no more has updated data. If you're working with it, you might end corrupting your data.

Anyhow, that's all I know :) @ajcvickers may provide more thorough information.