Efcore: Improve query cache eviction

[ajcvickers]

[ajcvickers]

See #12902, #8223

[craigpopham]

Just wanted to drop in and say this would be a high value feature for us. We've been chasing persistent memory leaks in some applications of ours running in Azure App Service,

We have some features which dynamically build queries, and this is causing the cache to explode in size. It was made worse by the fact that we had some instance members (on objects which contained the DBContext) being captured as in #8223, so the plan cache blew up in size really quickly.

We've been able to work around it, but improvements in this area would be welcome.

[ajcvickers]

Notes from discussion: we will use MemoryCache but fully internalized.

[AndriySvyryd]

We should consider using a simple heuristic to estimate the relative cache entry size (e.g. number of expressions in the tree).

[roji]

@AndriySvyryd when we do implement cache eviction, not sure we should try to track memory size as opposed to simply the number of queries cached... But we'll revisit...

[smitpatel]

For heuristic consider the cost of computing & usage count.

[NickCraver]

Unfortuntely, the changes in https://github.com/aspnet/EntityFrameworkCore/commit/67a200f68a1549d19560e9e6228daa411bd00a6c related to this issue are affecting other libraries. If EF Core is setup first (common) the rather low size limit added to the memory cache breaks things downstream, a la https://github.com/MiniProfiler/dotnet/issues/433 (breaking MiniProfiler's storage). If it's not setup first, this limit isn't even in place (given the TryAdd)...so it's not really a global solution to the issue anyway.

Has the impact of the startup race and the somewhat magical/hidden limits been considered here? I'm not sure the approach used is great for such a widely used service to be registered this way. Any thoughts on approaching this a different way?

[AndriySvyryd]

@NickCraver I thought this was fixed in https://github.com/aspnet/EntityFrameworkCore/commit/cbd28ff4871cc5abc626dc7b4917a0b20538e2f8#diff-1f53a7b867113cb4b9c61eccef38abd9L486, but that was a different change. This is definitely not the intended behavior.

[ajcvickers]

@NickCraver In 3.0, by default, the memory cache used by EF is independent of other memory cache instances precisely because of this issue. Concretely, this means AddDbContext no longer uses the memory cache setup by ASP.NET Core. However, the UseMemoryCache API still exists to optionally allow use of the same cache instance. Are you calling this? Is it necessary (or desired, within the limits of what IMemoryCache allows) to use the same cache for EF and other things?

Ideally, IMemoryCache would allow much better control over these kinds of things. However, the last time we revisited this with the ASP.NET team there was really no interest in going there.

[AndriySvyryd]

@ajcvickers I believe @NickCraver refers to the services added by the AddEntityFramework* methods. There are still ASP.NET apps that use them instead of just calling AddDbContext

[ajcvickers]

@NickCraver Are you calling AddEntityFramework or AddEntityFrameworkSqlServer? If so, for what reason?

[Cooksauce]

@ajcvickers The startup config which causes this issue is in the MiniProfiler issue he referenced. Specifically, this comment: MiniProfiler/dotnet#433-547077949

Are the AddEntityFramework methods not recommended? Is there documentation somewhere about why you would (or wouldn't) want to use them?

[ajcvickers]

@Cooksauce Yes, that is absolutely not recommended.

[AndriySvyryd]

Filed https://github.com/aspnet/EntityFrameworkCore/issues/19053 to help identifying this pattern.