c# GDI+, Creating a LinearGradientBrush in a loop (memory leaks)

Question

Dear colleagues.

Today I came across a bit of a dilema. I have created an app that uses GDI+ to draw on a form. The drawing is triggered by a timer every second. The draw method uses a for loop to iterate through a collection of objects and if they are of a certain state, draw them.

I want to draw them using a LinearGradientBrush simply because it looks so much nicer than a simple Brush. Have a look at the following

            //minutes
        foreach (Led l in MinuteGrid.Leds)
        {
            LinearGradientBrush b = new LinearGradientBrush
                (l.LedRectangle, Color.GreenYellow, Color.Green, 110);

            if (l.IsLit)
                g.FillRectangle(b, l.LedRectangle);

            b.Dispose();
        }

I am creating a new LinearGradientBrush for each iteration of the loop (which bothers me), but thats because I have to. I cannot create one outside the loop because its constructor set demands that I set parameters which are only ever known inside the loop.

I find that using the dispose method on the LinearGradientBrush object is not all that reliable. If I run my app and view it in Task manager, its spewing memory. When I then add the b = null line that seems to help hugely as follows

            foreach (Led l in MinuteGrid.Leds)
        {
            LinearGradientBrush b = new LinearGradientBrush
                (l.LedRectangle, Color.GreenYellow, Color.Green, 110);

            if (l.IsLit)
                g.FillRectangle(b, l.LedRectangle);

            if (b != null)
            {
                b.Dispose();
                b = null;
            }
        }

I am just wondering if there is a better way to work with LinearGradientBrushes ? Or is there a better solution to use ?

Many thanks

Answer 1

Add a gradient brush to each Led. If you can't add it to that class, then you could use a Dictionary<Led,GradientBrush> to store the brushes in to gain easy access to them. That way you only need one brush per Led instead of one per loop iteration,

(Also, in your example code, there is no point creating the brush if !l.IsLit)

Answer 2

I would recommend using a "using" statement:

foreach (Led l in MinuteGrid.Leds)
{
     if (l.IsLit)
     {
         using(LinearGradientBrush b = new LinearGradientBrush(l.LedRectangle, Color.GreenYellow, Color.Green, 110))
         {
            g.FillRectangle(b, l.LedRectangle);
         }
     }
}

However, remember, Dispose() does not free (managed) memory. It just releases the unmanaged resources (which is important, and may include unmanaged memory). The memory will not free until the GC runs, which may not happen during your loop.

However, if the memory pressure gets too high, the garbage collector should run within your loop, and you'll see it drop. This is the way .NET is designed - just accept it, and don't worry. The GC will eventually collect this memory, so its not something to worry about.

Answer 3

Dispose has nothing to do with freeing managed memory. That is handled entirely by GC, which runs "when needed". However, since the brush most likely holds a handle, you should dispose it. I would recommend that you do that in a using block instead of manually calling Dispose as this will make sure Dispose is called even in the presence of an exception.

Answer 4

If the number of permutations is limited you might just pre-create all your brushes once:

LinearGradientBrush rectGreenBrush = new LinearGradientBrush(l.LedRect........);
LinearGradientBrush rectRedBrush = new LinearGradientBrush(l.LedRect........);

foreach (Led l in MinuteGrid.Leds)
{
  LinearGradientBrush b = null;
  if (xxx)
    b = rectGreenBrush;
  else if (yyyy)
    b = rectRedBrush;
  else.....


  do painting
}

cleanup brushes

A second option is similar, but to create the brushes as needed;

List<LinearGradientBrush> createdBrushes = new List<LinearGradientBrush>();

foreach (Led l in MinuteGrid.Leds)
{
  LinearGradientBrush b = null;

  b = FindOrCreateBrushBasedOnLed(l, createdBrushes); 
  // if not already created, creates the brush and adds it to the list

  do painting
}

foreach (LinearGradientBrush b in createdBrushes)
{
  cleanup brushes
}

The other answers are correct that .NET may allow managed memory usage to balloon as long as it's not harming anything. But this should help cut out a lot of the creating/deleting if there are many Led objects to loop through.

Answer 5

I have now refactored to this

            foreach (Led l in MinuteGrid.Leds)
        {
            if (l.IsLit)
            {
                using (LinearGradientBrush b = new LinearGradientBrush
                    (l.LedRectangle, Color.GreenYellow, Color.Green, 110))
                {
                    g.FillRectangle(b, l.LedRectangle);
                }
            }

        }

However, still loosing 8-12KB in Task Manager every few seconds. Why is that ?