efcore icon indicating copy to clipboard operation
efcore copied to clipboard

IdentityMap still referenced in StateManager after DbContexts has been returned to DbContextPool

Open jonnybee opened this issue 2 years ago • 8 comments

Using EF Core 7.0.0-rc.2.22464.4 Database provider: Microsoft.EntityFrameworkCore.SqlServer IDE: VisualStudio 2022 v17.3.4 OS: Windows 11

Memory is still referenced for IdentityMap in StateManager after DbContexts have been returned to DbContextPool using DbContextFactory: image

After testing the same issue in release/6.0 this will change in StateManager.Clear method will fix the issue:

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

    .....

jonnybee avatar Sep 14 '22 17:09 jonnybee

As I wrote in the email offline, is that I'm not clear on why keeping the identity map instances is problematic. We do already clear them, which in turn clear _identityMap and _dependentMaps inside - that should mean no significant space is reachable any more. In fact, reusing the same identity map instance should be a good optimization, rather than continuously recreating them etc.

Any idea whether these instances actually mean a lot of space is being kept, and where that space is referenced from them?

roji avatar Sep 14 '22 17:09 roji

It is a bit weird as to why the objects are still kept in memory.

    private IIdentityMap? _identityMap0;
    private IIdentityMap? _identityMap1;
    private Dictionary<IKey, IIdentityMap>? _identityMaps;

    _identityMaps?.Clear();
    _identityMap0?.Clear();
    _identityMap1?.Clear();

But there is definitely a difference between calling Clear() on the Dictionary _identityMaps and Clear() on the instances of IdentityMap in _identityMap0 and _identityMap1 variables.

IdentityMap.Clear()

        public virtual void Clear()
        {
            _identityMap?.Clear();
            _dependentMaps?.Clear();
        }

vs System.Collections.Generic.Dictionary<TKey,TValue>

public void Clear()
{
	int count = _count;
	if (count > 0)
	{
		Array.Clear(_buckets);
		_count = 0;
		_freeList = -1;
		_freeCount = 0;
		Array.Clear(_entries, 0, count);
	}
}

jonnybee avatar Sep 14 '22 18:09 jonnybee

But there is definitely a difference between calling Clear() on the Dictionary _identityMaps and Clear() on the instances of IdentityMap in _identityMap0 and _identityMap1 variables.

Sure, but either way the StateManager is left referencing at most two IdentityMap instances (_identityMap0 and _identityMap1), which themselves have been cleared and so shouldn't be referencing anything. The _identityMaps Dictionary has been cleared and no longer references any IdentityMap.

So basically I'm not getting where the size is coming from here, and I'm wondering if it isn't a quirk of your memory profiler or similar.

roji avatar Sep 14 '22 18:09 roji

I used Visual Studio 2022 Performance Profiler - Memory Usage for the tests in the initial report.

Today I ran the same test application - but used dotnet-dump to dump the process and then open the dmp file in DotMemory.

And it still show's that _identityMap0 still has references to objects in memory. image

So, is it a quirk in the memory profiler? I belive not. Although I have no good explanation as to why the objects are still referenced and kept in memory I do know that setting the variables to null in Close method will make the GC collect the objects.

jonnybee avatar Sep 16 '22 12:09 jonnybee

This isn't making any sense to me, I'll try to reproduce this and investigate in the next few days.

roji avatar Sep 16 '22 13:09 roji

Is it just that a cleared dictionary does not (by design) free up all its cleared memory?

Array.Clear(_buckets);
_count = 0;
_freeList = -1;
_freeCount = 0;
Array.Clear(_entries, 0, count);

Notice that the Array.Clear methods do not release the arrays to be garbage collected, but instead instead clear their values. This means that a dictionary that is cleared saves on garbage collection and re-allocations when it is re-used.

So the question here is probably, "is the dictionary likely to be re-used?" If so, then it's probably best to use Clear, and this is in fact the reasoning for using Clear here in the EF Code. On the other hand, if it's not going to be re-used, then it makes sense to null out the reference and let the whole structure get collected. But then, if it is re-used after all, there will be extra overhead re-initializing.

ajcvickers avatar Sep 18 '22 12:09 ajcvickers

Does it make sense that the _entries arrays alone make up that much space? I'd also expect to see them separately in the memory profiler as the source of the size, rather than the IdentityMap type (though maybe they're being folded).

roji avatar Sep 18 '22 13:09 roji

Does it make sense that the _entries arrays alone make up that much space?

I don't know; haven't investigated an actual running instance.

I'd also expect to see them separately in the memory profiler

Agreed.

ajcvickers avatar Sep 18 '22 15:09 ajcvickers

@jonnybee I've looked at this again, and I still can't see how this makes sense... As @ajcvickers wrote above, we do clear the dictionaries referenced by the StateManager. Dictionary.Clear itself does Array.Clear on Dictionary's two internal arrays, which should release any objects referenced by them - but does not decrease the size of the arrays themselves; however, I can't imagine that being the source of the size.

To make sure we're not missing something, can you submit the code sample you're using to repro this? I have a code snippet from you in the email, but it isn't full (e.g. the model is missing); if you can submit a minimal console program, I'll investigate using that.

roji avatar Sep 24 '22 18:09 roji

@roji Will do but I'm on travel and conference this week so will send you code next week,

jonnybee avatar Sep 27 '22 11:09 jonnybee

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.

ajcvickers avatar Oct 12 '22 17:10 ajcvickers