12
\$\begingroup\$

I have a problem with my printing program, in which it uses up too much resources while printing (to paper or to .pdf). I try to manually dispose as much as I can, but for example, when I tried printing 300 pages, the program itself used up around 500mb of memory, and I would like to avoid that. When the program finishes printing, it goes back down to 37mb of usage. I use Visual Studio to check performance. Below you can find the code, and any help to manage resources and lower ram usage while printing would be appreciated. I apologize in advance for mixing German and English. And for clarity, this draws n number of each element (one is a string, second an arrow, up or down, third a barcode).

private void DocumentDrucker_PrintPage(object sender, System.Drawing.Printing.PrintPageEventArgs e)
{
    Graphics graphic = e.Graphics;
    SolidBrush brush = new SolidBrush(Color.Black);

    Font font = new Font("Courier New", 27, FontStyle.Bold);

    float pageWidth = e.PageSettings.PrintableArea.Width;
    float pageHeight = e.PageSettings.PrintableArea.Height;

    float fontHeight = font.GetHeight();
    int startX = 40;
    int startY = 40;
    int offsetY = 40;

    for (; elemente < ZumDrucken.Items.Count; elemente++)
    {
        graphic.InterpolationMode = System.Drawing.Drawing2D.InterpolationMode.NearestNeighbor;
        graphic.DrawString(ZumDrucken.Items[elemente].Text, font, brush, startX, startY + offsetY);

        if (ZumDrucken.Items[elemente].Checked == true)
        {
            if (ZumDrucken.Items[elemente].Text.Substring(ZumDrucken.Items[elemente].Text.Length - 1) != "1")
                graphic.DrawImage(Properties.Resources.pfeilU, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
            else
                graphic.DrawImage(Properties.Resources.pfeilO, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
        }
        else
        {
            if (ZumDrucken.Items[elemente].Text.Substring(ZumDrucken.Items[elemente].Text.Length - 1) != "1")
                graphic.DrawImage(Properties.Resources.pfeilO, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
            else
                graphic.DrawImage(Properties.Resources.pfeilU, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
        }

        b.Encode(TYPE.CODE128A,ZumDrucken.Items[elemente].Text, Color.Black, Color.Transparent,600,100);
        graphic.DrawImage(b.EncodedImage, new Point(Convert.ToInt32(pageWidth*0.6),offsetY));

        offsetY = offsetY + 175;               

        if (offsetY >= pageWidth-100)
        {
            e.HasMorePages = true;
            offsetY = 0;
            elemente++;

            graphic.Dispose();
            b.Dispose();
            brush.Dispose();
            font.Dispose();

            return;
        }
        else
        {
            e.HasMorePages = false;
        }
    }
    graphic.Dispose();
    b.Dispose();
    brush.Dispose();
    font.Dispose();

}
\$\endgroup\$
14
\$\begingroup\$
  • don't mix german and english words for names. Best is to stick to english because most/all developers knows the language.
  • don't repeat yourself. You have some duplicated code which should be removed.

    e.g:

    You create the same Point for printing at 4 different location (new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37))

  • don't omit braces {} although they might be optional. Omitting them can lead to hidden and therfore hard to track bugs.


Let's take a look at the if..else construct and how we could refactor it

if (ZumDrucken.Items[elemente].Checked == true)
{
    if ZumDrucken.Items[elemente].Text.Substring(ZumDrucken.Items[elemente].Text.Length - 1) != "1")
        graphic.DrawImage(Properties.Resources.pfeilU, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
    else
        graphic.DrawImage(Properties.Resources.pfeilO, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
}
else
{
    if (ZumDrucken.Items[elemente].Text.Substring(ZumDrucken.Items[elemente].Text.Length - 1) != "1")
        graphic.DrawImage(Properties.Resources.pfeilO, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
    else
        graphic.DrawImage(Properties.Resources.pfeilU, new Point(Convert.ToInt32(pageWidth * 0.35), offsetY + 37));
}  

First we can remove the == true because comparing a non nullable bool to true is senseless. Either it is true or false.

Now let us create a

var imagePoint = new Point(Convert.ToInt32(pageWidth * 0.35), offsetY);  

just before the loop and increase the Y before the if..else we are talking about so the whole thing becomes

imagePoint.Y += 37;
if (ZumDrucken.Items[elemente].Checked)
{
    if (ZumDrucken.Items[elemente].Text.Substring(ZumDrucken.Items[elemente].Text.Length - 1) != "1")
    {
        graphic.DrawImage(Properties.Resources.pfeilU, imagePoint);
    }
    else
    {
        graphic.DrawImage(Properties.Resources.pfeilO, imagePoint);
    }
}
else
{
    if (ZumDrucken.Items[elemente].Text.Substring(ZumDrucken.Items[elemente].Text.Length - 1) != "1")
    {
        graphic.DrawImage(Properties.Resources.pfeilO, imagePoint);
    }
    else
    {
        graphic.DrawImage(Properties.Resources.pfeilU, imagePoint);
    }
}

If we extract the checking of the last character from the text outside of the if..else like so

bool lastCharIsAOne =  ZumDrucken.Items[elemente].Text[ZumDrucken.Items[elemente].Text.Length - 1] == '1';  

we don't create a new string because we just access the char array directly.

But for readability I would introduce a

var currentItem = ZumDrucken.Items[elemente]; 

then the if..else will become

var currentItem = ZumDrucken.Items[elemente]; 
... some more code

bool lastCharIsAOne =  currentItem.Text[currentItem.Text.Length - 1] == '1';  
imagePoint.Y += 37;
if (currentItem.Checked)
{
    if (lastCharIsAOne)
    {
        graphic.DrawImage(Properties.Resources.pfeilU, imagePoint);
    }
    else
    {
        graphic.DrawImage(Properties.Resources.pfeilO, imagePoint);
    }
}
else
{
    if (lastCharIsAOne)
    {
        graphic.DrawImage(Properties.Resources.pfeilO, imagePoint);
    }
    else
    {
        graphic.DrawImage(Properties.Resources.pfeilU, imagePoint);
    }
}  

Now we need to do something about the image which should be printed. We see that pfeilU should be printed if:

(currentItem.Checked && lastCharIsAOne) || (!currentItem.Checked && !lastCharIsAOne) which is the same as (currentItem.Checked == lastCharIsAOne)like @DDrmmr stated in the comments, hence we only need one if and one else which we could reduce to a simple if like so

var currentImage = Properties.Resources.pfeilO;

if (currentItem.Checked == lastCharIsAOne)  
{
    currentImage = Properties.Resources.pfeilU;  
}  
graphic.DrawImage(currentImage, imagePoint);

If we now extract the setting of the graphic.InterpolationMode outside of the loop we will get this

graphic.InterpolationMode = System.Drawing.Drawing2D.InterpolationMode.NearestNeighbor;

var imagePoint = new Point(Convert.ToInt32(pageWidth * 0.35), offsetY);  
var barcodePoint = new Point(Convert.ToInt32(pageWidth * 0.6), 0);  

for (; elemente < ZumDrucken.Items.Count; elemente++)
{
    var currentItem = ZumDrucken.Items[elemente]; 

    graphic.DrawString(currentItem.Text, font, brush, startX, startY + offsetY);

    var currentImage = Properties.Resources.pfeilO;

    bool lastCharIsAOne =  currentItem.Text[currentItem.Text.Length - 1] == '1';  
    if (currentItem.Checked == lastCharIsAOne)  
    {
        currentImage = Properties.Resources.pfeilU;  
    }

    imagePoint.Y += 37;  
    graphic.DrawImage(currentImage, imagePoint);

    b.Encode(TYPE.CODE128A, currentItem.Text, Color.Black, Color.Transparent, 600, 100);

    barcodePoint.Y = offsetY;
    graphic.DrawImage(b.EncodedImage, barcodePoint);

    offsetY = offsetY + 175;  

    ... the other `if..else`
\$\endgroup\$
  • 1
    \$\begingroup\$ don't mix german and english words for names While I agree that you should try to avoid it, I have also come across situations that make it acceptable. If there is a well-known name (e.g. leefloonsubsidie) that is self explanatory (because it's commonly used in your project/business), that does not mean that the direct English translation is equally unambiguous, well-known or similarly self explanatory. In those cases of established nomenclature, I find it acceptable to keep using the foreign language for brevity's sake. E.g; GetLeefloonSubsidie() or SaveLeefloonsubsidie(). \$\endgroup\$ – Flater May 4 '17 at 14:59
  • \$\begingroup\$ @Flater for such cases it could be ok, but here if the german words would be translated to english would be clear as well. \$\endgroup\$ – Heslacher May 4 '17 at 15:03
  • \$\begingroup\$ Fair enough. My example is always limited to business logic concepts (I think), not local variable names or similarly "easy to give" names. \$\endgroup\$ – Flater May 4 '17 at 15:05
  • \$\begingroup\$ While I would like to thank you for cleaning up my code, this does not increase performance. Even after calling b.EncodedImage?.Dispose();, it uses about the same amount of RAM as the code in my original post. Also, the code imagePoint.Y += 37; should be imagePoint.Y = offsetY + 30;, otherwise the arrows are only separated by those 37, since the imagePoint is outside of the loop. \$\endgroup\$ – Einherjar May 5 '17 at 6:01
  • \$\begingroup\$ if ((currentItem.Checked && lastCharIsAOne) || (!currentItem.Checked && !lastCharIsAOne)) can be written more succinctly as if (currentItem.Checked == lastCharIsAOne). \$\endgroup\$ – D Drmmr May 5 '17 at 10:34
20
\$\begingroup\$

I try to manually dispose as much as I can

This is usually good however this time you dispose too much.

private void DocumentDrucker_PrintPage(object sender, PrintPageEventArgs e)
{
    Graphics graphic = e.Graphics;

    ..

    graphic.Dispose(); // Don't!    
}

You receive the Graphics object via the PrintPageEventArgs. This means that you should not dispose it. The owner takes care of the graphics object. You just use it for drawing. I wonder that this works at all because as soon as the graphics is disposed the DocumentDrucker has nothing to print. This should actually crash.

\$\endgroup\$
  • \$\begingroup\$ Don't quote me on this, but I think it is because the last call of the graphic.Dispose(); is outside of the for loop. If the for loop does come through, it doesn't matter, as all the pages that have already been printed, but if there are several pages to print, it goes back to the top of the printing method, and a new Graphics object is created. \$\endgroup\$ – Einherjar May 5 '17 at 6:07
  • \$\begingroup\$ @einherjar, more or less. Inside loop will fail for sure, outside loop it doesn't because a new drawing context (Graphics object) is created for each printed page (your code is executed for each page) then no one else will use that context after you disposed it. Still it's something you should not do! \$\endgroup\$ – Adriano Repetti May 5 '17 at 6:42
11
\$\begingroup\$

Do not manually call Dispose(), you will end-up having some duplication (and you're not even disposing those resources in case of errors...). Use using():

using (var graphics = new Graphics())
{
    // Your code here...
}

Also do not create a new Font for each page, reuse the same one for all pages. Maybe it's time to move this code into its own separate class with its instance fields?

You do not need to create a SolidBrush(Color.Black), just use Brush.Black. Is if (offsetY >= pageWidth-100) correct? You're comparing Y offset with page width.

Note that I suppose elemente and b are instance fields, you might want to make it clear: this.elemente and this.b or rename them to _b and _elemente. Also b isn't best name for an object might it be barcodeImageConverter (guessing from parameters), it's hard to understand what it does without reading the full-code and thinking about it. Also elemente may be something like latestPrintedElement.

Get rid of all those hard-coded constants, they will make your code a nightmare to maintain. Use private const int (or float) for them and you will also gain in readability.

I'd say that memory consumption come from b.Encode() method but we don't have any idea about what it is and what it does. Does it create a bard-code image for the given text? An educate guess may be that you need to manually dispose images generated by b.EncodedImage after you used them and before you create a new one calling Encode():

b.EncodedImage?.Dispose();
b.Encode(TYPE.CODE128A, ...

All this said...to fix memory issues nothing is better than a memory profiler...

\$\endgroup\$
-1
\$\begingroup\$

It seems that some bitmap is being generated here. It might be causing memory pressure as it is inside a loop.

b.Encode(TYPE.CODE128A,ZumDrucken.Items[elemente].Text, ...
graphic.DrawImage(b.EncodedImage...

Try adding GC.Collect() at the end of event handler and see if consumption of memory is reduced.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.