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

#460 - Memory leak fix. #474

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@heXelium
Contributor

heXelium commented Feb 9, 2016

I investigated issue described in the provided thread. It's easy to reproduce it by creating Project in a following way (this code is copied from Roslyn and than modified to show the point):

public async Task Foo()
{
     string path = @"...some project link";
     var properties = new Dictionary<string, string>();

     for (int i = 0; i < 100; i++)
     {
          var xmlReader = XmlReader.Create(await ReadFileAsync(path, CancellationToken.None).ConfigureAwait(false), s_xmlSettings);
          var collection = new ProjectCollection();
          var xml = ProjectRootElement.Create(xmlReader, collection);
          xml.FullPath = path;

          var project = new Project(
                    xml,
                    properties,
                    toolsVersion: null,
                    projectCollection: collection);
     }
}

Whole memory will be consumed by cached items in ProjectStringCache. There no mechanism of freeing cache data automatically if the references to instances of ProjectCollection and Project classes will be lost. The only way to handle this is to call ProjectCollection.UnloadProject() or ProjectCollection.UnloadAllProjects() manually.

It seems that in Roslyn they ended up with some kind of a workaround by wrapping Project in ProjectFile class with destructor:

~ProjectFile()
{
     try
     {
          // unload project so collection will release global strings
          _loadedProject.ProjectCollection.UnloadAllProjects();
     }
     catch
     {
     }
}

So solution that I propose is pretty much simple. I just implemented Dispose pattern for ProjectCollection to clear all this stuff on GC.

@AndyGerlicher

This comment has been minimized.

Member

AndyGerlicher commented Feb 11, 2016

This looks great, and thank you for putting in the effort to try to fix it.

However, it also looks very scary! Particularly the comment:

/// Because of the strongly typed list, some ProjectRootElement's will be held onto indefinitely. This is an acceptable price to pay for
/// being able to provide a commonly used ProjectRootElement immediately it's needed. It is mitigated by the list being finite and small, and
/// because we allow ProjectCollection.UnloadAllProjects to hint to us to clear the list.

Because we don't know the implications, this is definitely out of scope for Update 2 (or probably 3) unless we have a really solid understanding of 1) why is this list not finite and small as suggested and 2) what implications calling ProjectCollection.UnloadAllProjects will have when in a long running process (like Visual Studio IDE) that may still need project references even after the finalizer may run. This change also seems to undo a lot of the logic defined in the ProjectRootElementCache, and it looks like a lot of thought went into it. Is there an issue with the weak/strong reference implementation? Would it be better simply without the cache or that particular implementation? Without knowing the answer to those questions, we can't take this change for VS2015. We would like to have this issue addressed in Dev15 though, so I will leave this open until we can do some further analysis.

@heXelium

This comment has been minimized.

Contributor

heXelium commented Feb 12, 2016

@AndyGerlicher thank you for quick and clear answer. Despite my fix currently affects ProjectRootElementCache the real issue is in ProjectStringCache. So I tried to find a safer solution. And after doing more testing I see some option how to resolve this.
ProjectStringCache purpose is to intern strings for loaded XmlDocuments to use less memory. But in fact it uses more than need. See below screenshots from memory profiler. I executed example code provided above to load same project file 1 and 100 times in a loop.
image
image
On second screenshot you can see that strings is really interned but it also keeps references to every instance of created XmlDocument (even loaded xml doc is same). The reason of this because XmlDocument.GetHashCode() implementation return hashcode of object instance, it's not overridden.
When I removed string internation for testing purposes, obviously memory leak was resolved. And also it works faster while loading and unloading projects simultaneously.
So the issue can be resolved quickly by removing internation and as second option override XmlDocumentWithLocation.GetHashCode() to compare xml entirely. Not sure what is better option here from perf perspective. Both of them have drawbacks. It's no clear for me why do we need keep references to XmlDocument if instances of Project and ProjectCollection already collected.
What do you think about it? Can we go with one of them? Or you have other thoughts.

@Sarabeth-Jaffe-Microsoft

This comment has been minimized.

Contributor

Sarabeth-Jaffe-Microsoft commented Jun 21, 2016

@heXelium Thanks for looking in to this! For now, we're going to close this pull request since we don't have a clear direction on this fix.

@OniBait

This comment has been minimized.

OniBait commented Jan 3, 2017

I looked into this and ran into a similar situation where Visual Studio ends up taking more and more memory due to this.

Taking memory snapshots I'm seeing where 500MB of memory was being used purely by the ProjectStringCache._documents dictionary.

The problem is indeed that the projects aren't getting unloaded and ejected from this cache properly and the XmlDocument (used as the key) still has a reference to it.

So the end result is that every build causes a memory leak -- with build imports like Microsoft.Common.CurrentVersion.targets utilizing 247k, that adds up pretty quickly (and completely destroys the benefit of interning the strings).

I think a better solution might be to use XmlDocumentWithLocation.FullPath as the key to the dictionary (and not interning strings where that is null to account for when the project hasn't been loaded from disk) or to use something like ConditionalWeakTable<> to fix this huge memory leak.

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