Dispose SoundEffect and Texture2D (mainly RenderTarget2Ds) properly #860

Merged
merged 1 commit into from Oct 15, 2012

Projects

None yet

3 participants

@Nezz
Member
Nezz commented Oct 12, 2012

...from destructor to prevent memory leaks

@KonajuGames
Contributor

There is a lot of Dispose behaviour to fix up across the codebase.

@tomspilman
Member

This looks like a good fix/improvement to me.

Still i really don't recommend depending on finalizers to clean up hardware resources... you will end up running out of GPU memory and the C# GC will not know how to recover from that.

You really need to be calling Dispose() to release GraphicsResource objects.

@KonajuGames
Contributor

Nezz and I have had that discussion before.

@Nezz
Member
Nezz commented Oct 12, 2012

I try to fix what I can. At least now I know that I should not be looking for a single leak, but dozens of them.

@KonajuGames
Contributor

Please call Dispose on IDisposable objects, as you should be[1][2]. Then
you will be looking for far less leaks and be a better developer for it.

[1]
http://cs.rthand.com/blogs/blog_with_righthand/archive/2007/06/18/To-Dispose-or-not-to-Dispose.aspx
[2]
http://stackoverflow.com/questions/45036/will-the-garbage-collector-call-idisposable-dispose-for-me

@Nezz
Member
Nezz commented Oct 12, 2012

This is totally offtopic, but Microsoft's recommended pattern is:

class X:  IDisposable
{
   public X(…)
   {
   … initialize resources … 
   }

   ~X()
   {
   … release resources … 
   }

   public void Dispose()
   {
// this is the same as calling ~X()
        Finalize(); 

// no need to finalize later
System.GC.SuppressFinalize(this); 
   }
};

We can not rely on people calling dispose for every single object. Dispose is mostly used by those who want C++ destructors, but just because of that, we should not be allowing memory leaks in the code.

We decided not use dispose because our game design allows us to use a full GC collects between levels, which makes sure we won't get collected during gameplay.

@tomspilman
Member

I try to fix what I can

I think it is totally valid to properly add finalizers to the disposables, but we need to do them right...

        protected virtual void Dispose(bool disposing) 
        {
            if (disposed)
               return;

             // DISPOSE RESOURCES HERE!

             disposed = true;

             // Suppress finalization
             if (disposing)
                 GC.SuppressFinalize(this);
        }

Calling suppress is something recommended when you have a type with unmanaged resources like we do with all the GraphicsResources. See...

http://msdn.microsoft.com/en-us/library/ms182329(v=vs.110).aspx

@tomspilman
Member

Our comments crossed in the cloud. :)

Dispose is mostly used by those who want C++ destructors,

That is wrong for this case.

Dispose is used with XNA because you cannot depend on the GC to free unmanaged resources.

Trust that if Dispose() wasn't required for XNA graphics types... they wouldn't be in XNA. They are their because they should be used.

Still proper usage of Dispose() is to implement finalizers as well.

@tomspilman
Member

One last thing from the horse's mouth on XNA and disposable...

Garbage collection is great for making code simpler and easier to write, but it isn't so good at handling managed wrappers around native resources such as textures or vertex buffers. The garbage collector doesn't control the underlying native objects: all it sees are the tiny managed wrappers. It is easy to get into a situation where your graphics card is struggling under the weight of hundreds of megabytes of texture data, but the garbage collector is thinking "ho hum, everything fine here, they did allocate this array of a hundred Texture objects, but that's ok, each Texture is only a few bytes and I have nearly a megabyte free still, so there's no point bothering to collect anything just yet".

http://blogs.msdn.com/b/shawnhar/archive/2006/09/06/743437.aspx

@Nezz
Member
Nezz commented Oct 12, 2012

I never said that disposable is a bad thing, just that we should not rely on it so much that we don't put cleanup code in the destructor :)

@tomspilman
Member

I think we're saying the same thing then.

Dispose() is correct and the right way you should be freeing your objects.

If you happen to forget to dispose something we still should have a proper finalizer in place to collect the managed resource and not leak it.

@KonajuGames KonajuGames merged commit 827f3da into MonoGame:develop3d Oct 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment