Tuesday, May 17, 2011

The insidious nature of unmanaged resource leaks in XNA games.

While working on Star Ninja's screen transition system last week, I discovered a memory leak problem that was ultimately determined to be the result of a simple oversight - The instanced model system wasn't disposing the vertex buffers and index buffers it had created. These are created at runtime to prepare large arrays that are a series of duplicates of the master instance data with the bone indices and index indices set up to do the SkinnedEffect instancing technique as described in one of the official XNA samples. Finding that out was a lot more time consuming than I'd have liked. The only reason I noticed it was because of another bug that had sized the vertex buffer allocation incorrectly and when I fed a larger mesh into the system, memory problems started to show up.

While I do most development and testing on a PC, mainly for the time saving benefit of "edit and continue" which isn't available on WP7 or XBox, I do make a point of running and doing a few quick tests after every significant task to make sure everything is still working well. Recently, I added a conditional compilation symbol "STRESS_TEST" which allows me to just run the game and it will churn through all the levels doing pretty random stuff. The tester is a great way for me to monitor for peak memory usage; the code will periodically print out the peak memory retrieved from Microsoft.Phone.Info.DeviceExtendedProperties. Since certification requires the game to stay under 90MB, this is a pretty important thing to stay on top of.

After the last batch of changes where one small part included changing which assets were being fed into the instanced object renderer, I saw the memory usage spike unexpectedly and almost immediately over 100MB and more as time went on. So I ran the game using a CLR memory profiler (YourKit for .NET) which to my surprise wasn't telling me anything useful - object counts were pretty similar and managed heap was about the same. This suggested to me that the memory usage being reported by WP7 is process memory and not just the managed heap. While interesting to know this, it doesn't help in finding what was sucking all the extra memory.

Because the asset change was just one change of what was probably a few too many, I didn't immediately recognize that as being related to the problem. Because my usual reference-leak-finding technique weren't working, I found myself disabling large swaths of code to narrow down the source of the unmanaged memory leak. After a few hours of divide & conquer, I eventually I found the problem was due to these vertex & index buffers not being disposed. The code was simply not releasing these unmanaged resources and neither the CLR or the XNA API had any way to know that something needed to be cleaned up. With this failure now understood, I reviewed all the code for anything that might need a call to Dispose(), called it at the appropriate time, and the unmanaged memory leaks disappeared.

In the end, a lot of time was wasted due to not paying enough attention to objects implementing the IDispose interface. Sufficiently chastised by this, I made a point of reviewing the Dispose Pattern in case it would help avoid this in the future. I had glossed over it before but had avoided using it because C#'s limitation that classes can only derive from one other class made me leery of introducing that kind of limitation to the general codebase without a good reason. Most of the time, interfaces provide the desired results with only a little more work and without the single-derivation limitation and so using interfaces rather than derivation had been my typical approach. This memory leak situation provided the motivation required to start using the dispose pattern in the hopes that future errors could be avoided.


To my surprise, I soon found there isn't a standard Disposable base class in .NET. Why not? Perhaps because it's so simple that people just write them whenever needed? Who knows. Simpler classes seem to be provided on a regular basis, but that's how it is.

So here's one you may find slightly more useful than the basic Disposal pattern.


/// <summary>
 /// A generic implementation of the Dispose pattern, useful for classes that need IDisposable, don't need to 
 /// derive from something else, and are used as a base class for other classes.
 /// </summary>
 public class Disposable : IDisposable
 {
  /// <summary>
  /// Set to true as soon as Dispose is called and before the
  /// call to Dispose(true) is made, which means this bool is 
  /// only useful to code outside the scope of the disposal process.
  /// </summary>
  public bool IsDisposed { getprivate set; }
 
  public void Dispose()
  {
   if(IsDisposed)
   {
       throw new Exception();
   }
   IsDisposed = true;
   Dispose(true);
   GC.SuppressFinalize(this);
  }
 
  protected virtual void Dispose(bool disposing)
  {
  }
 
  ~Disposable()
  {
   Dispose(false);
  }
 }


This has one feature beyond the standard Dispose pattern - a bool that is set when it is disposed, which can be a useful debug build assertion in code that uses the object, particularly when an object is being bounced around among various systems and detecting a disposed object can prevent more mysterious exceptions at a lower level. Also, I couldn't think of a situation where it would be useful to call Dispose on an object twice, so the check for IsDisposed in Dispose() will help find situations where this somehow happens by accident.

The bool IsDisposed could perhaps be made visible to #DEBUG builds, and the check converted to a Debug.Assert(), but I prefer to have these failures detected at the earliest time in all configurations in order to prevent other, possibly more subtle, bugs from occurring. While it's true that an object that has had Dispose() called upon it can technically continue to be used, I prefer to consider Disposed objects to be "off limits" where I expect them to be inert and ready for garbage collection so I often check the IsDisposed property at entry points to large systems just to make sure an object is still valid.

After reviewing the code for all IDispose interfaces and converting all suitable classes to derive from the new Disposable base, I found the code in general was a little more organized (especially class hierarchies where multiple classes in the hierarchy implemented IDispose) and generally more robust in that I knew the class finalizers would be taking care of any disposals that, for whatever reason, weren't explicitly triggered.

While the Disposal class is all well and good, the main thing to remember is to always make sure to pay attention to the objects you create and if they implement IDisposable, make sure it's getting called at some point because unlike most of C# there isn't any magic code that will clean it up for you. The Disposable class doesn't eliminate the need to actually write the code to dispose the objects you are responsible for, but it can make your library code a little more robust when you provide a Disposable as a return value because this way the finalizer will at least be sure to eventually dispose the object in case the caller doesn't.