Skip to content
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

Excessive memory usage after enabling DbContextFactory #28988

Closed
jonnybee opened this issue Sep 6, 2022 · 13 comments · Fixed by #28989
Closed

Excessive memory usage after enabling DbContextFactory #28988

jonnybee opened this issue Sep 6, 2022 · 13 comments · Fixed by #28989
Assignees
Labels
area-perf area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Milestone

Comments

@jonnybee
Copy link

jonnybee commented Sep 6, 2022

Using EF Core 6.0.5 and Microsoft.Data.SqlClient 4.1.0 running in Azure AppService and SqlAzure database.

After enabling DbContextFactory we se excessive memory usage in our application.

The context pool is as default (up to 1024 DbContexts) and there is a number of VARCHAR(MAX) columns in our database.
When a DbContext is return to pool the SqlConnection (and BufferedDataReader) will still hold a large amount of memory from the last SQL Query.

Am I doing something wrong or shouldn't this be cleared from the pooled DbContext?

DbContextPool_1

DbContextPool_2

@roji
Copy link
Member

roji commented Sep 6, 2022

Yeah, thanks for flagging this.

We reuse the same RelationalDataReader instance across RelationalCommand invocations, since EF 6.0 (part of the query optimizations). RelationalDataReader maintains a reference to its DbDataReader, which is provided each time Initialize is called; but that reference is only removed when the same instance of RelationalDataReader is reused for another query. This isn't a problem for e.g. SqlDataReader, but it's a problem for BufferedDataReader. And since RelationalDataReader is cached all the way to the RelationalConnection, we potentially have 1024 pooled contexts, each with its own RelationalDataReader referencing a BufferedDataReader.

We should clear out the reference to the DbDataReader when RelationalDataReader is disposed.

@jonnybee
Copy link
Author

jonnybee commented Sep 6, 2022

I'm wondering about the disposal of _currentResultSet i BufferedDataReader.

The memory dump show that the memory is referenced in _currentResultSet and this is not disposed/cleared when BufferedDataReader is Closed / Disposed:

BufferedDataReader.cs:

    public override void Close()
    {
        _bufferedDataRecords = null!;
        _isClosed = true;

        var reader = _underlyingReader;
        if (reader != null)
        {
            _underlyingReader = null;
            reader.Dispose();
        }
    }

    protected override void Dispose(bool disposing)
    {
        if (!_disposed
            && disposing
            && !IsClosed)
        {
            Close();
        }

        _disposed = true;

        base.Dispose(disposing);
    }

Shouldn't _currentResultSet variable also be disposed/set to null?

@roji
Copy link
Member

roji commented Sep 6, 2022

My approach in #28989 was simply to sever the reference from RelationalDataReader (which is recycled and therefore referenced from pooled DbContext instances) to DbDataReader. Unless I'm missing something, that means that BufferedDataReader and all of its referenced data (including _currentResultSet) should become eligible for garbage collection once the query's enumerator is disposed.

@jonnybee
Copy link
Author

jonnybee commented Sep 6, 2022

Yes, that will work for PooledDbContext but if you have a long living DbContext the _currentResultSet variable will not be cleared until the next query is executed.

Maybe no problem.

@roji
Copy link
Member

roji commented Sep 6, 2022

Yes, that will work for PooledDbContext but if you have a long living DbContext the _currentResultSet variable will not be cleared until the next query is executed.

Why is that? Context pooling shouldn't be relevant here; the moment a query enumerator is disposed (the query finishes evaluating), RelationalDataReader.Dispose() is called, and #28989 that sets RelationalDataReader._reader to null. At this point there should be no more references to the BufferedDataReader and the GC can reclaim it.

Or are you seeing something different in the code?

@jonnybee
Copy link
Author

jonnybee commented Sep 6, 2022

No, my mistake from not being so familiar with the internals of EF Core.

@roji
Copy link
Member

roji commented Sep 6, 2022

No problem, it's great to have another pair of eyes on this.

@roji
Copy link
Member

roji commented Sep 7, 2022

Reopening to consider for servicing.

@roji roji reopened this Sep 7, 2022
roji added a commit to roji/efcore that referenced this issue Sep 7, 2022
roji added a commit to roji/efcore that referenced this issue Sep 7, 2022
@roji
Copy link
Member

roji commented Sep 7, 2022

@jonnybee I've merged #28989 for 7.0 and submitted #29005 for considering for 6.0.

Can you please try out the latest 7.0 daily build and confirm that it fixes the issue for you?

@roji roji added the area-query label Sep 7, 2022
@jonnybee
Copy link
Author

First of all - I have verified that the fix in main is good for EF Core 7.

I have tried to create private build from main (release/7.0) and it does compile fine but the assemblies, although compiled for .NET 6, references Microsoft.Extensions.* v7.0.0 and then fails on assembly load at runtime.

I have then done further memory profiling with private builds of code in release/6.0 branch and have found on more issue in StateManager:

As-Is: after DbContexts has been returned to pool:
image

Then changed StateManager.Clear() method to set IdentityMap(s) properties to null:

public virtual void Clear()
{
    Unsubscribe();
    ChangedCount = 0;
    _entityReferenceMap.Clear();
    _referencedUntrackedEntities = null;
 
    _identityMaps?.Clear();
    _identityMaps = null!;
    _identityMap0 = null!;
    _identityMap1 = null!;

and I get this result:
image

Which shows that IdentityMaps have now been collected and cleared from memory.

@ajcvickers
Copy link
Member

I have tried to create private build from main

You shouldn't need to build yourself, rather just use the daily build feed.

have found on more issue in StateManager:

Thanks! we'll look into this.

@jonnybee
Copy link
Author

I verified the fix from the daily build feed but tried to test changes in Clear method in StateManager by creating a private build and that failed on assembly load at runtime in my test program.

@roji
Copy link
Member

roji commented Sep 14, 2022

@jonnybee can you please open a new issue for the above so we can track it separately from the original issue?

@ajcvickers ajcvickers added this to the 6.0.x milestone Sep 16, 2022
@ajcvickers ajcvickers modified the milestones: 6.0.x, 6.0.11 Sep 26, 2022
@ajcvickers ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed Servicing-consider labels Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants