New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible memory leak? #10535

Closed
joshmouch opened this Issue Dec 11, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@joshmouch

joshmouch commented Dec 11, 2017

Sorry that this is a vague question, but I'm facing a memory leak of some kind, and when I do a memory dump and examine it, almost everything stuck in memory is related to EntityFramework. The highest memory usage is from Microsoft.Extensions,Caching.MemoryCache, which looks like it's hanging on to a lot of CacheEntry's containing instances of Func<QueryContext, IEnumerable<OneOfMyEntites>> (8.8 MILLION, to be exact).

Does anything pop out at you as to why this might be? I'm pretty sure I'm not hanging on to any instances of anything EF-related in between requests.

Here's a dump of the entire memory pool:

image

@ajcvickers

This comment has been minimized.

Member

ajcvickers commented Dec 13, 2017

@joshmouch What version of EF Core are you using? (We fixed some issues in the 1.x timeframe.) Beyond that, we will likely need a runnable project/solution or code listing that reproduces the issue.

@ajcvickers

This comment has been minimized.

Member

ajcvickers commented Dec 20, 2017

EF Team Triage: Closing this issue as the requested additional details have not been provided and we have been unable to reproduce it.

BTW this is a canned response and may have info or details that do not directly apply to this particular issue. While we'd like to spend the time to uniquely address every incoming issue, we get a lot traffic on the EF projects and that is not practical. To ensure we maximize the time we have to work on fixing bugs, implementing new features, etc. we use canned responses for common triage decisions.

@joshmouch

This comment has been minimized.

joshmouch commented Mar 21, 2018

@ajcvickers
I finally had a bit more time to look into this more.
In answer to your question on version, I'm using 2.0.2.

Correct me if I'm wrong here, but I believe "proof" of the memory leak can be shown by adding a check to DbContext constructor to assert there is nothing in the MemoryCache for a newly instantiated DbContext from a newly-created DI scope. So when I add the following code, it results in an exception in between requests:

public class MyDbContext : DbContext

    protected MyDbContext(....) {
		var compiledQueryCache = ((Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache)this.GetService<Microsoft.EntityFrameworkCore.Query.Internal.ICompiledQueryCache>());
		var memoryCache = ((MemoryCache)typeof(Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache).GetTypeInfo().GetField("_memoryCache", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(compiledQueryCache));
		
		if (memoryCache.Count > 0) throw new Exception("DbContext has a memory leak.");
	}
}

This is the code you'd run to get the exception:


using (var scope = this.ServiceProvider.CreateScope())
{
	var serviceProvider = scope.ServiceProvider;

	var dbContext = serviceProvider.GetRequiredService<MyDbContext>();
	var entity = dbContext.Entities.Skip(1).Take(1).First();
}

using (var scope = this.ServiceProvider.CreateScope())
{
	var serviceProvider = scope.ServiceProvider;

	// THE NEXT LINE THROWS EXCEPTION
	var dbContext = serviceProvider.GetRequiredService<MyDbContext>();
	var entity = dbContext.Entities.Skip(1).Take(1).First();
}
@smitpatel

This comment has been minimized.

Contributor

smitpatel commented Mar 21, 2018

@joshmouch - MemoryCache is registered as singleton in DI hence shared across scope so the code you posted above will throw by design. Though since you are running the same query again, the cache size would remain at 1.

@joshmouch

This comment has been minimized.

joshmouch commented Mar 21, 2018

@smitpatel , @ajcvickers
Makes sense. So maybe the memory leak is caused because the MemoryCache is looking at the same query twice and thinking that they're different? In my screenshot above, I think that's saying that there are 21,600 cached query plans for the same entity type!!!!!
I use dynamically built expressions quite a bit to pass in to EF. Maybe that's causing an issue?

@smitpatel

This comment has been minimized.

Contributor

smitpatel commented Mar 21, 2018

@joshmouch - That's certainly possible that if EF Core fails to identify 2 same queries as one then Cache would grow even though not required. If you can identify example of such query where the same query would cause Cache to grow, we can investigate the cause.

@joshmouch

This comment has been minimized.

joshmouch commented Mar 21, 2018

@smitpatel

I found the code that causes this. Here's what I'm doing:

I create an Expression (lots of steps to create this):
var filter = System.Linq.Expressions.Expression<System.Func<MyEntity, bool>>

When you do a ToString on the Expression it looks like this:
{e => (Convert(e.PrimaryKey) == e121e002-b749-4933-b229-4c2d1ca49be7)}

Then I get a DbSet like so:
var dbSet = dbContext.Set<MyEntity>();

I then create an IQueryable
var query = dbSet.Where(filter);

var results = query.ToList();

Every time that GUID changes, it causes a new cached plan to be created.
 

@xt0rted

This comment has been minimized.

xt0rted commented Mar 21, 2018

I'm not using EF Core yet, but in EF6 I noticed my queries weren't being parameterized when doing paging and advanced filtering so they weren't using the same/cached execution plans. Section 4.2 at https://msdn.microsoft.com/en-us/library/hh949853(v=vs.113).aspx explains how you need to use lambdas and other techniques to force parameterization in a lot of areas. Once I started making those changes my queries started being parameterized and would share the same execution plan. Is this what's going on here? Do the queries need to be adjusted so they're parameterized which will then reduce the amount of in-memory cache?

@joshmouch

This comment has been minimized.

joshmouch commented Mar 21, 2018

@xt0rted
I'm thinking along similar lines. However, the type that dbSet.Where() expects is Expression<Func<TEntity, bool>>.

So hopefully there's a slight modification I can make to the Expression to cause EF to realize that Expression.Constant ('e121e002-b749-4933-b229-4c2d1ca49be7') should not be part of the cached plan.

Any ideas here, @smitpatel ?

@smitpatel

This comment has been minimized.

Contributor

smitpatel commented Mar 21, 2018

@xt0rted - That article was true for the case of EF6. Precisely due to that issue, we improved parametrization in EF Core. So even if you use Skip/Take & other operations which does not take lambda arg, we would still generate parameters. So no work-around needed from user side in EF Core for such cases.

@joshmouch - While constants without lambda are parametrized, when constants are used inside lambda expression, they are not. In some cases the query may be same but generally it is non-trivial to have same query plan since constant expression inside lambda is part of bigger expression. If you want to use parameter inside lambda expression then you need to provide a parameter/variable. Following example will help you understand what happens with parametrization in EF Core.

// In following queries constant is inlined so we do the same in SQL and generate inline literal.
// Hence SQL is different and we compile query twice.
// Notice that query had to be constructed fully for both times.
query = dbSet.Where(e => e.PK == "Value").ToList();
query = dbSet.Where(e => e.PK == "AnotherValue").ToList();

// In following queries constant value is passed through a variable from closure. So we generate a parameter for it.
// Same SQL can be re-used with different parameter value.
// Notice how we could use single Queryable with change of variable value
var filterValue = "Value";
query = dbSet.Where(e => e.PK == filterValue);
query.ToList();

filterValue = "AnotherValue";
query.ToList();

For your case you would need to change ConstantExpression to actually be variable coming in from block then it would avoid memory leak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment