4
\$\begingroup\$

I'm making a top-down shoot-em-up in XNA, and I have placeholder blocks for my characters and bullets. Here's a picture:

image1

I have all the Bullets stored in a List<Bullet>, such that a Bullet is deleted whenever it collides with the window bounds. So I have a List<Bullet> called bullets, and I call bullets.RemoveAt(i) to delete a Bullet object.

However, if I keep on holding down Space to shoot Bullets, my "Mango Warfare" application starts "bleeding bullets," or bleeding memory. The app starts to climb up the memory rankings in Windows Task Manager:

image 2

Here's my code for removing out-of-window-bounds bullets:

private void CheckBulletCollisions() { foreach (Bullet bullet in bullets) { if (bullet.X > GraphicsDevice.Viewport.Width || bullet.X < -5 || bullet.Y > GraphicsDevice.Viewport.Height || bullet.Y < -10) { bullet.Enabled = false; continue; } } } private void RemoveDisabledBullets() { int num_bullets = bullets.Count; int count = 0; int i = 0; while (count < num_bullets) { if (!bullets[i].Enabled) { bullets.RemoveAt(i); ++count; continue; } ++count; ++i; } } 

Clearly, RemoveAt() is not working. Is there anything else I can do to free up memory for out-of-window-bounds bullets?

\$\endgroup\$
4
  • \$\begingroup\$ For which platform are you developing? PC, XBox, WP7? \$\endgroup\$ Commented May 23, 2012 at 19:57
  • \$\begingroup\$ Just PC at the moment. \$\endgroup\$ Commented May 23, 2012 at 19:59
  • 1
    \$\begingroup\$ You're looking at the private working set of the process in the task manager there; it's not a very reliable metric for diagnosing memory issues like this. RemoveAt() works fine, and presuming there are no other outstanding references to your bullet objects (whether or not this is true isn't something that can be determined from just this code, unfortunately) they will eventually be reclaimed by the GC. \$\endgroup\$ Commented May 23, 2012 at 21:24
  • \$\begingroup\$ As far addressing a solution, there are a number of ways you can have better control over the memory consumption of your bullets, but some of them depend on information you haven't provided. Do you have a fixed upper limit to the number of bullets you'll want on the screen at any one time? \$\endgroup\$ Commented May 23, 2012 at 21:27

6 Answers 6

11
\$\begingroup\$

Don't use the task manager to do memory profiling. It doesn't mean what you think it means, especially for C# or other managed applications. ANTS Memory Profiler is a wonderful managed memory profiler that I've used repeatedly. It is not free, but it does have a trial. As Jonathan Dickinson points out in the comments, the CLR itself also exposes several performance counters you can view in perfmon.exe, including several related to .NET memory and garbage collection.

There is a very good chance there's nothing actually rooting your bullet objects once they are removed from that list and that, upon a subsequent collection of the appropriate garbage generation, the memory consumed by the objects will be reclaimed. Without seeing more of your code I can't be sure, but I think it's a safe bet.

However, you are doing something that is, if nothing else, inefficient and you could improve it:

You're removing items from a list, which implies you are allocating them and putting them in the list in the first place -- in other words you are allowing allocation to occur during your game loop, and this means you'll eventually cause garbage collection (and more to the point, possibly cause more frequent collection of higher GC generations). Further, you're removing items from the middle of the list (most of the time), which means every time you remove something, the following elements of the list must be copied.

Consider instead partitioning your list into two halves, an "active" and a "disabled" half. You will need an additional integer variable somewhere to track the index of the first disabled bullet in the list.

When your game starts, you preallocate some reasonable number of bullets in the list and set the "first disabled" index to 0 (because all bullets in the list are currently disabled). Whenever you need to "create" a new bullet you just set the properties of bullets[firstDisabled] to whatever values the new bullet should have:

bullets[firstDisabled].X = desiredX; bullets[firstDisabled].Y = desiredY; bullets[firstDisabled].Speed = 100; // The bullet is now active: ++firstDisabled; 

In your main update loop, you only process bullets from 0 to firstDisabled:

for (var bulletIndex = 0; bulletIndex < firstDisabled; ++bulletIndex) { // Update the bullet here. } 

To handle the case where a bullet has left the screen during the update loop, you don't remove it from the list. Instead you swap that bullet's data with the last enabled bullet's data (which is at firstDisabled - 1 and decrement firstDisabled.

Obviously this technique implies two things: you have a fixed upper limit of live bullets and that the order of bullets in the list doesn't matter. The latter should not be a problem, however the former might be. Fortunately you can deal with this if you have to by adding a new bullet at the end of the list (not in the middle) and swapping that bullet's properties with the bullet at index firstDisabled. Then you increment firstDisabled -- much like how you handle "removing" a disabled bullet, really.

The end result is that you have a system that allows bullets to be activated and deactivated in linear time (your original method was more intensive) and has a fixed memory budget which can't leak or induce garbage collection during gameplay and is easily cleaned up post-gameplay by simply clearing the entire list (if need be).

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Also known as object pooling, good answer. +1 \$\endgroup\$ Commented May 23, 2012 at 21:55
  • \$\begingroup\$ I changed the List<Bullet> to a 10-bullet array after reading some of the other answers, but I was still allocating objects and assigning these objects to array locations during the game loop. I'll have to give your advice a try, thanks! \$\endgroup\$ Commented May 23, 2012 at 22:13
  • 1
    \$\begingroup\$ +1. You might also want to look into adding perf counters into your answer: the .Net ones can give you an estimate of the memory usage (without the speed impact of profiling). \$\endgroup\$ Commented May 24, 2012 at 8:35
4
\$\begingroup\$

2 Things. First, it could be some kind of garbage collection issue. Just because you are removing the Bullet from the List doesn't mean it or some part of it still doesn't exist in memory. I don't know enough about the specifics or how Bullet is implemented, but it could be the Bullet object is creating something (A Texture2d?) and not properly destroying it?

2nd, For the loop in your second code, you have two variables (count and i) that grow exactly the same. Just pointing that out. See Hackerworth's method in the other answer.

Hope this is helpful! -Woody

\$\endgroup\$
6
  • \$\begingroup\$ It seems you're right Woody, the bug smells the memory leak due to not deleted bullets after being removed from the list. \$\endgroup\$ Commented May 23, 2012 at 20:18
  • \$\begingroup\$ If OP comes back and shows some more of his Bullet code, we might be able to help better. I'm no C# expert. \$\endgroup\$ Commented May 23, 2012 at 20:27
  • 2
    \$\begingroup\$ Your for() loop is bugged - after it removes a bullet, it will not check the new bullets[count], which is the old bullets[count+1], so in the worst case, if all bullets are disabled, it will remove only half of them. \$\endgroup\$ Commented May 23, 2012 at 20:29
  • \$\begingroup\$ I think it will remove all of bullets - because of "continue" statement. \$\endgroup\$ Commented May 23, 2012 at 20:39
  • 1
    \$\begingroup\$ The loop should work I think since i is not incremented when a bullet was removed, but it's definately a contrieved way of doing it. It would be much better to iterate through the list from the end. \$\endgroup\$ Commented May 23, 2012 at 21:14
2
\$\begingroup\$

First, you don't need 2 indices i and count, one will suffice.

For clarity, I would rewrite the method:

private void RemoveDisabledBullets() { int i = bullets.Count - 1; while (i >= 0) { if (!bullets[i].Enabled) { bullets.RemoveAt(i); } --i; } } 

Second, I have no reason to believe that RemoveAt(i) somehow doesn't work, it's a very basic method. Maybe there's a bug in your while loop, it's certainly convoluted enough to allow for that possibility, no offense, but if you call RemoveAt(i) and your program does not throw an exception, then the element at i has indeed been removed from that list.

If you are absolutely positive that the bullets are causing your memory leak, then the reason must be that your bullets are still being referenced somewhere else and are not getting garbage-collected.

Can you confirm that bullets is really the only active object that references bullets?

I haven't personally tried it, but have a look here to log whether or not the garbage collector has actually run.

\$\endgroup\$
1
  • \$\begingroup\$ A for loop would be even more clear. \$\endgroup\$ Commented May 23, 2012 at 21:32
1
\$\begingroup\$

Is continue even necessary? Not seeing how the program would get caught in that one line of the if statement.

I'm guessing RemoveDisabledBullet() is called in your Update method after the CheckBulletCollisions() method

maybe try a for loop. It's worked for me in the past.

for (int x = bullets.Count - 1; x >= 0; x--) { if (!bullets[x].Enabled) { bullets.RemoveAt(x); } } 
\$\endgroup\$
1
\$\begingroup\$

I know this is like the third or so answer with this suggestion, but I would rewrite your for loop like this:

private void RemoveDisabledBullets() { for (int i = 0; i < bullets.Count;) { if (!bullets[i].Enabled) { bullets.RemoveAt(i); } else { i++; } } } 

This makes sure that count gets evaluated for every step and that every bullet is actually checked to see if it is disabled. It's how I do it for my own bullets. :)

\$\endgroup\$
0
\$\begingroup\$

Is it possible that even tho removeAt() is removing the reference to the object that the operating system is not reporting it as free (and may not report it as free until it decides to claim it for another activity)?

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.