Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Dispose SoundEffect and Texture2D (mainly RenderTarget2Ds) properly #860

Merged
merged 1 commit into from

3 participants

@Nezz

...from destructor to prevent memory leaks

@KonajuGames
Owner
@tomspilman
Owner

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
Owner
@Nezz

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
Owner
@Nezz

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
Owner

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
Owner

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
Owner

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

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
Owner

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 from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 12, 2012
  1. @Nezz

    Dispose SoundEffect and Texture2D (mainly RenderTarget2Ds) properly f…

    Nezz authored
    …rom destructor to prevent memory leaks
This page is out of date. Refresh to see the latest.
View
22 MonoGame.Framework/Desktop/Audio/SoundEffectInstance.cs
@@ -74,6 +74,11 @@ public SoundEffectInstance()
InitializeSound();
}
+ ~SoundEffectInstance()
+ {
+ Dispose();
+ }
+
public SoundEffectInstance (SoundEffect parent)
{
this.soundEffect = parent;
@@ -109,13 +114,16 @@ void HandleSoundBufferReserved (object sender, EventArgs e)
}
public void Dispose ()
- {
- this.Stop(true);
- soundBuffer.Reserved -= HandleSoundBufferReserved;
- soundBuffer.Recycled -= HandleSoundBufferRecycled;
- soundBuffer.Dispose ();
- soundBuffer = null;
- isDisposed = true;
+ {
+ if (!isDisposed)
+ {
+ this.Stop(true);
+ soundBuffer.Reserved -= HandleSoundBufferReserved;
+ soundBuffer.Recycled -= HandleSoundBufferRecycled;
+ soundBuffer.Dispose();
+ soundBuffer = null;
+ isDisposed = true;
+ }
}
public void Apply3D (AudioListener listener, AudioEmitter emitter)
View
5 MonoGame.Framework/Graphics/Texture2D.cs
@@ -257,6 +257,11 @@ public Texture2D(GraphicsDevice graphicsDevice, int width, int height)
{
}
+ ~Texture2D()
+ {
+ Dispose();
+ }
+
public int Width
{
get
Something went wrong with that request. Please try again.