Memory Leak after for loop when displaying previous draw line in C#

Question

When I am drawing line and then iterate in for loop it cause high memory leak then after several line it crashed.

private Pen ReDrawFromGrid(Pen pen)
    {
        for (int parseDgV = 0; parseDgV < dataGridView1.Rows.Count; parseDgV++)
        {
            float redrawX1;
            float redrawY1;
            float redrawX2;
            float redrawY2;
            float.TryParse(dataGridView1.Rows[parseDgV].Cells[0].Value.ToString(), out redrawX1);
            float.TryParse(dataGridView1.Rows[parseDgV].Cells[1].Value.ToString(), out redrawY1);
            float.TryParse(dataGridView1.Rows[parseDgV].Cells[2].Value.ToString(), out redrawX2);
            float.TryParse(dataGridView1.Rows[parseDgV].Cells[3].Value.ToString(), out redrawY2);
            if (dataGridView1.Rows[parseDgV].Cells[5].Value.ToString() == "Solid")
            {
                dashRedraw = new float[1] { 10 };
            }
            else if (dataGridView1.Rows[parseDgV].Cells[5].Value.ToString() == "Dash")
            {
                dashRedraw = new float[2] { 10, 10 };
            }
            else if (dataGridView1.Rows[parseDgV].Cells[5].Value.ToString() == "Dot")
            {
                dashRedraw = new float[2] { 3, 5 };
            }
            else
            {
                dashRedraw = new float[1] { 10 };
            }
            pen = new Pen(dataGridView1.Rows[parseDgV].Cells[4].Style.BackColor);
            pen.DashPattern = dashRedraw;
            g.DrawLine(pen, redrawX1, redrawY1, redrawX2, redrawY2);
            this.Refresh();
            this.axDNVideoX1.Invalidate();
        }
        GC.Collect();
        return pen;
    }

I tried to put dispose before and after in this for loop but it resulted no line drawn. How can I iterate draw line without causing memory leak?

Thanks for your help!

Answer 1

First, the parameter "pen" does not have sense to me. You are passing a Pen to your method and then in your method you are creating a new instance of the Pen and then you are returning a Pen. I would cache all required Pen instances and reuse them. Once you don't need a particular Pen any more, Dispose it.

The only 'Memory leak' worrying part of the code here that I see is a 'axDNVideoX1' which is it seems an ActiveX control and it has to do something with Video. Try to comment out that part of the code and see if you still have a memory leak.

I would not say that the 'DrawLine' causes a memory leak.

Keep your eye on the count of the 'GDI objects' created and destroyed. You can see this value in the Task Manager.

Answer 2

You are creating a Pen per loop iteration. You need to dispose those objects properly. The pen you pass to the function is lost, the pens you create in the loop are lost except for the last which is returned.

using blocks are your friend. Use them.

Answer 3

Here's a quick re-write of the basic drawing code with some minor tweaks:

static float[] patSolid = new float[] { 10 };
static float[] patDash = new float[] { 10, 10 };
static float[] patDot = new float[] { 3, 5 };

private void RedrawFromGrid(Graphics g)
{
    float X1, Y1, X2, Y2;
    for (int i = 0; i < dataGridView1.Rows.Count; i++)
    {
        var cells = dataGridView1.Rows[i].Cells;

        if (!float.TryParse(cells[0].Value.ToString(), out X1) ||
            !float.TryParse(cells[1].Value.ToString(), out Y1) ||
            !float.TryParse(cells[2].Value.ToString(), out X2) ||
            !float.TryParse(cells[2].Value.ToString(), out Y2)
        )
            continue;

        var style = cells[5].Value.ToString();
        float[] pattern = patSolid;
        if (style == "Dash")
            pattern = patDash;
        else if (style == "Dot")
            pattern = patDot;

        using (var pen = new Pen(cells[4].Style.BackColor))
        {
            pen.DashPattern = pattern;
            g.DrawLine(pen, X1, Y1, X2, Y2);
        }
    }
}

It's a bit dirty still, since I wanted to keep the basic functionality close to where you had it.

A few refinements:

The only thing that needs to be allocated inside the loop is the pen object, and you could get rid of that too by caching the pens and reusing them. But since we're doing allocation each time, the pen needs to be disposed of when you're done with it. The using statement does this for you. You should be doing the same for the Graphics object in the method that calls this.

Second: don't allocate the same arrays over and over, allocate them once and reuse them. You already know ahead of time what patterns you are going to be using, so pre-allocating the pattern arrays is a good idea. Especially if this is getting called 30 times per second by your video code or similar.

And finally, this method should have as few side effects as possible. The method that calls it should take care of the side effects - calling this.Refresh() or this.axDNVideoX1.Invalidate() after disposing of the Graphics object when all the drawing is done.

In general a drawing operation should be as fast - and have as few side effects - as it is possible to make it. Allocating arrays, calling Invalidate or Refresh, etc. are for the caller to do. Don't let your drawing method do anything but draw.

---

edit:

Incidentally, calling GC.Collect() will not solve a memory leak. In fact it's quite likely that it's not going to do anything at all in this context, since none of the allocations happening inside this loop will have reached termination anyway. When you find yourself directly invoking the garbage collector it is almost always a mistake.