POP QUIZ: What’s wrong with this code – part 3


Imagine you are a developer and your boss comes to you complaining that your piece of code has been deemed to be taking up too much memory and causing problems for the application.  You take a look at your code and you see the following, assume that stream is defined above this and is correct:

const String myConstStr = "CurrentFile";
const String stylesheet = "..\\..\\XSLTFile1.xslt";
int i = 0;
 
for(i = 0; i < 1000; i++)
{
    XslTransform xslt = new XslTransform();
    xslt.Load(stylesheet);
 
    //Load the XML data file.
    String filename = myConstStr + i.ToString() + ".xml";
    XPathDocument doc = new XPathDocument(filename);
 
    //Create an XmlTextWriter to write to the stream.         
    XmlTextWriter writer = new XmlTextWriter(stream);
    writer.Formatting = Formatting.Indented;
 
    //Transform the file.
    xslt.Transform(doc, null, writer);
    writer.close();
}

So what is wrong with this snippet of code?  (hint, there may be more then one thing that can be fixed).

Comments (66)

  1. sudeepg says:

    The code would leak memory because we are loading XSLT multiple times within the loop. This would create temporary assemblies that cannot be unloaded until the appdomain is restarted.

    The resolution to this is in KB: 316775

    Another solution maybe to allow creating and loading these assemblies in a separate appdomain and unload that appdomain.

  2. Mifko says:

    1) Do not create XslTransform in each loop. Move these 2 lines of code out of the for loop:

    XslTransform xslt = new XslTransform();

    xslt.Load(stylesheet);

    The same applies to XmlTextWritter if that writes to the same stream in each iteration of the loop. If this is the case then do not close it before next iteration but after the loop instead.

    2) Do not concatenate strings using the + operator, use string.Format or StringBuilder instead

    string filename = string.Format("{0}{1}.xml", myConstStr, i);

    So the code should look like this, shouldn’t it?

    const String myConstStr = "CurrentFile";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    int i = 0;

    XslTransform xslt = new XslTransform();

    xslt.Load(stylesheet);

    //Create an XmlTextWriter to write to the stream.        

    XmlTextWriter writer = new XmlTextWriter(stream);

    writer.Formatting = Formatting.Indented;

    for(i = 0; i < 1000; i++)

    {

       //Load the XML data file.

       String filename = String.Format("{0}{1}.xml", myConstStr, i);

       XPathDocument doc = new XPathDocument(filename);

       //Transform the file.

       xslt.Transform(doc, null, writer);

    }

    writer.close();

  3. Anatoliy says:

    Hi!

    I didn’t test my answer, but on the first look I have to move out independent objects from for loop. This objects are XslTransform and XmlTextWriter. If we declare and initialize its before loop we save some emomory.

  4. James says:

    Why would you want to transform an xml file a thousand times in the first place?!

  5. Jay says:

    XSLTransform/XmlTextWriter  instantiation needs to be moved outside the loop.. a 1000 instances are being created

  6. Daniel Klingstedt says:

    The line "String filename = myConstStr + i.ToString() + ".xml";" takes a lot of memory. You should use StringBuilder-class when stringconcatenating.

  7. Henk de Koning says:

    I’d use the XslCompiledTransform, load the xslt once and share the instance across requests. There is some error handling that can be added, strings that can be formatted instead of glued together, plus you can probably save some (de)serialization by using the Transform overload that takes the input filename and output stream. But that’s about it.

    The resource problem is caused by a) creating a new XslTransform per request and b) the way XslTransform interpreted the stylesheet (the compiled version compiles into a temp assembly I think). If I’m not mistaken XslTransform is [Obsolete]d, yet another reason to precompile your sites ;-).

    BTW, creating a new XslCompiledTransform per request would result in code bloat due to the amount of assemblies loaded. Or does it use dynamic methods ?

    — Henkk

  8. GP says:

    1. The XslTransform shall be created and initialized (.Load) outside of the loop.

    2. A compiled Transform shall be used

  9. Michael says:

    My guess is that because stream is declared outside the scope of the for loop, it contains all data written to it during the scope of the loop. So every time it loops through the for loop the result of the transformation is stored in the stream.

    Also in case of an exception the object writer will never be closed.

  10. Chuck D says:

    For starters, move the load of the xsl and writer out of the for loop.

    Release your xpathdocument references when you are done with them.

  11. manta says:

    No need to load the xsl inside the loop, just initialize it outside the loop.

  12. Here is the correct code(i think) says:

    const String myConstStr = "CurrentFile";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    int i = 0;

    XslTransform xslt = new XslTransform();

    xslt.Load(stylesheet);

    for(i = 0; i < 1000; i++)

    {

       //Load the XML data file.

       String filename =String.Format("{0}{1}.xml", myConstStr , i)

       XPathDocument doc = new XPathDocument(filename);

       //Create an XmlTextWriter to write to the stream.

       XmlTextWriter writer = new mlTextWriter(stream);

       writer.Formatting = Formatting.Indented;

        //Transform the file.

       xslt.Transform(doc, null, writer);

       writer.close();

    doc=null;

    }

  13. Mark says:

    I would first suggest having the XLST load be outside the loop so you arent creating 1000 wasted instances

  14. Denis says:

    loading the same stylesheet 1000 times comes to mind.

    So does creating/closing the writer the same number of times.

  15. Steve says:

    I’m new to this but if I had to guess I’d say the code creates 1000 instances of an XslTransform that loads the same stylesheet when only one instance is needed to transform the 1000 xml files and should be declared and loaded outside the for-loop.

  16. evilweeble says:

    Surely there’s no need to load a XslTransform  stylesheet 1000 times??

  17. mhildreth says:

    It looks like there’s no reason why the xslt object couldn’t be declared outside the loop since it never changes.

  18. Wayne says:

    Well I guess that

    XslTransform xslt = new XslTransform();  

    xslt.Load(stylesheet);

    Could be created outside of the loop so that it isn’t being created 1000 times, also I would write

    string filename = string.concat(myConstStr, i, ".xml") to avoid creating extra string references in each pass.

    I could be miles off but worth a shot 😉

  19. Wayne says:

    Couldn’t you also create the XmlTextWriter outside of the loop as it is only creating the same thing over and over again if stream has already been defined and isn’t going to change during the loop….

  20. jungalist says:

    I’m going to go with: The document is being loaded for each iteration of the loop.

  21. ElFruitcake says:

    Move XslTransform xslt = new XslTransform();    xslt.Load(stylesheet);

    out of the 1000 loop…

    and not sure about the other!

  22. Darrell says:

    The XSLTFile1.xslt is being loaded on every iteration of the loop.  Load it once before the loop.

  23. Jason says:

    Taking a few shots at it:

    1. Shouldn’t you be .Dispose()ing the XmlTextWriter object when you’re done with it?

    2. You only need to declare the XslTransform object once (and .Load() it as well).

    That’s all I got.

  24. Joao Carlos Clementoni says:

    concat inside for not using StringBuilder?

  25. Ben says:

    The first thing that jumps out is that the code isn’t disposing the writer.

    A second look says that you could promote the XslTransform to a higher scope and only load it one time…

  26. Aloneplayer says:

    cod line String filename = myConstStr + i.ToString() + ".xml";

    , new  XPathDocument(), new XPathDocument() and new new XmlTextWriter

    used in 1000 times loop create lots of temporary objects and consume memory.

  27. chris says:

    dont check that the file actually exists beforehand

    should be in try-catch blocks to catch errors

  28. darknode says:

    My variant is:

    const String myConstStr = "CurrentFile";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    for (int i = 0; i < 1000; i++)

    {

    /// Class XslTransform is obsolete, use XslCompiledTransform

    XslCompiledTransform xslt = new XslCompiledTransform();

    xslt.Load(stylesheet);

    //Load the XML data file.

    String filename = myConstStr + i.ToString() + ".xml";

    XPathDocument doc = new XPathDocument(filename);

    /// Before: XmlTextWriter writer = new XmlTextWriter(stream);

    /// stream valiable not found.

    using (MemoryStream stream = new MemoryStream())

    {

    using (XmlTextWriter writer = new XmlTextWriter(stream, Encoding.UTF8))

    {

    writer.Formatting = Formatting.Indented;

    xslt.Transform(doc, null, writer);

    /// Before: writer.close();

    /// After: writer.Close();

    /// But not used 🙂

    }

    }

    }

  29. Ashic says:

    1. First two lines in the for loop…the transform is loaded on every iteration. Move it before the loop. Why on earth would you…. 🙂

    2. Same case with the XMLTextWriter. We’re creating and closing it on every iteration. No need.

  30. I guess you don’t need to new-up all these objects in the loop and rather put them outside the loop scope and reuse them.

    Regards,

    Klaus

  31. Hazel says:

    From first glance of the code snippet, I found the following:

    1) Instantiating and loading the XSLT can be done before the loop.

    2) Similarly, the XMLTextWriter can also be instiantied before the loop and we can close it at the end of the code snippet (after the loop)

    3) We can use regurlar XMLDocument instead of XPathDocument to load the data files within the loop

  32. Matt Brooks says:

    I think I spot a few things:

    1) The loop counter variable should be declared as part of the FOR-loop, e.g. for (int i = 0; …

    This allows the C# compiler to optimise the compiled output in that it doesn’t have to bounds-check the loop variable on each iteration. However, I don’t think this would cause memory problems.

    2) The document loaded into the XslTransform instance should only be loaded once – in the example it is loaded 1000 times.

    3) There will be a lot of String instances created by concatenating the strings that are assigned to the ‘filename’ variable. Approximately 3000? These will hang around until the garbage collector kicks in. Perhaps String.concat() or String.format() would be a better option?

    4) The XmlTextWriter instance isn’t being disposed. This might be consuming resources (i.e. memory) until the object is finalised by the garbage collector.

  33. Johnny says:

    "XslTransform xslt = new XslTransform();

    xslt.Load(stylesheet);"

    This piece of code is loading the Style for each iteration in the loop.  This can be located outside before the loop starts.

    Also the XMLTextWriter can be defined, set and close outside the loop.

  34. Matt Brooks says:

    5) Ooops… The XmlTextWriter doesn’t have to be instantiated on every loop. The XslTransform output could simply be written to the same open XmlTextWriter stream on each iteration. Therefore this point kind of replaces (4).

  35. redsquare says:

    Well as a starter for 10 loading the same xslt 1000 times i not good, should cache the first transform object and re-use this

  36. Michael says:

    Nice questions! I just discovered your blog and read through the old trivia questions.

    For this one, I’d say that:

    1. The XSLT stylesheet (and the respective XslTransform) is static and should only be loaded once.

    2. Same for the XmlTextWriter, which we shouldn’t close after each document (if I understand the code correctly, in that it wants to write all the output into ONE result document).

    3. XslTransform is outdated and XslCompiledTransform should be used instead.

    4. If we wanted to conserve memory, we might use another class implementing IXPathNavigable (e.g. XmlDocument) that does not perform as much preparation for XPath queries.

  37. Jorge Salinas says:

    1.- These two lines must be outside of the for loop so it be instatiated one time instead of 1000

    XslTransform xslt = new XslTransform();

    xslt.Load(stylesheet);

    2.- The string filename can be a StringBuilder and use the Append Method to use less memory.

    3.- A similar optimization made in 1, can be used to    

    XmlTextWriter writer = new XmlTextWriter(stream);

    writer.Formatting = Formatting.Indented;

    This can be outside the for loop and the writer.close() outside the for loop, this assuming stream is the same stream for the whole loop.

    I’m not sure about this anyway, it would be wise to measure the optimization number 3. I’m pretty sure about the 1 and 2.

    Thanks.

  38. csudhg says:

    xslt should be loaded only once. outside the loop. I don’t see any other problem ;o

    const String myConstStr = "CurrentFile{0}.xml";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    XslTransform xslt = new XslTransform();

    xslt.Load(stylesheet);

    XPathDocument doc;

    for (int i = 0; i < 1000; i++)

    {

       String filename = string.Format(myConstStr, i);

       doc = new XPathDocument(filename);    

       using (XmlTextWriter writer = new XmlTextWriter(stream))

       {  

           writer.Formatting = Formatting.Indented;

           xslt.Transform(doc, null, writer);

       }

    }

  39. Josh says:

    Dispose() not called on writer.

  40. MZ says:

    Do you need to use String builder to build the filename? I know using String Builder will save some time. What other optimization can be done?

  41. Joe says:

    The stylesheet is being loaded within the for loop

    "XslTransform xslt = new XslTransform();"    

    "xslt.Load(stylesheet);"

    Since the same stylesheet is being loaded repeatedly, it can be loaded outside the loop

  42. Javier Sanchez says:

    – the lines defining and loading xslt should be outside the for statement.

    – dispose writer

  43. mYsZa says:

    After first glimpse, here is what I think:

    1. move the lines:

       XslTransform xslt = new XslTransform();

       xslt.Load(stylesheet);

    before the loop start.

    2. Create filename using string.Format() or StringBuilder

    3. I am not sure about this, but shouldn’t the XmlStreamWriter be wrapped in "using" statement? That would not only close it, but also dispose after use.

  44. tprzeliorz says:

    1.

       XslTransform xslt = new XslTransform();        

       xslt.Load(stylesheet);

    Give this outside "for" -> why 1000 times declare and load XSLT file

  45. Vaidyanathan Alagappan says:

    The declaration and loading part should be outside the loop

    XslTransform xslt = new XslTransform();    xslt.Load(stylesheet);

  46. Marc Sherman says:

    This line creates 3 seperate strings each time through the loop: String filename = myConstStr + i.ToString() + ".xml";

    I’ve read that using a StringBuilder handles memory a lot better since there’s only a single string in memory vs. the 3 above.

  47. Carlos Lone says:

    The first problem is with string concatenation inside a loop and problably the second thing is that you’re creating alot of objects of type XPathDocument  and XmlTextWriter which i’m not sure but could use unmanaged resources so the best thing is to use Using statement to ensure Idisposable.

  48. Dale Hirt says:

    Well, the first and obvious answer is you should only instantiate one instance of XmlTextWriter.  Since you are writing over and over to it, creating 1000 instances of it is a big memory hog.

    Second, the same thing can be said for the XslTransform object.  You only need one instance, not 1000.

    Next? 🙂

  49. d2eason says:

    How often is the stream actually flushed?  Something unobvious like writer.close() not actually flushing the stream could mean the results of all 1000 transforms are in memory.  (I didn’t really research.  Just guessing…)

  50. vadmyst says:

    I can outline several problems

    1) Relative path of stylesheet variable. The code can run under diferrent working folders. So it may result in strange behavior and sporadic FileNotFound exceptions

    2)     XslTransform xslt = new XslTransform();

       xslt.Load(stylesheet);

    can be moved out of the for loop. This can positively  affect performance.

    3) didn’t see the stream variable initialization.

  51. viet says:

    These two line below should be keep out of the loop statement.

    XslTransform xslt = new XslTransform();    

    xslt.Load(stylesheet);

  52. The first place I’d start is to load the XSLT outside of the loop, since it’s unncessarily being done in each pass.

    Still looking for some other fixes…

  53. ren forever says:

    const String myConstStr = "CurrentFile";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    int i = 0;

    for(i = 0; i < 1000; i++){

       XslTransform xslt = new XslTransform();

       xslt.Load(stylesheet);

        //Load the XML data file.    String filename = myConstStr + i.ToString() + ".xml";    XPathDocument doc = new XPathDocument(filename);     //Create an XmlTextWriter to write to the stream.             XmlTextWriter writer = new XmlTextWriter(stream);    writer.Formatting = Formatting.Indented;     //Transform the file.    xslt.Transform(doc, null, writer);    writer.close();}

  54. beibeilong says:

    const String myConstStr = "CurrentFile";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    int i = 0;

    for(i = 0; i < 1000; i++)

    {    

    XslTransform xslt = new XslTransform();    

    xslt.Load(stylesheet);     //Load the XML data file.    String filename = myConstStr + i.ToString() + ".xml";    

    XPathDocument doc = new XPathDocument(filename);     //Create an XmlTextWriter to write to the stream.            

    XmlTextWriter writer = new XmlTextWriter(stream);    writer.Formatting = Formatting.Indented;     //Transform the file.    xslt.Transform(doc, null, writer);    writer.close();}

  55. 1. The XslTransform should be instantiated and loaded outside the loop.

    2. Where is stream defined?

    3. If xslt.Transform throws an exception, writer is not closed properly => using(…) { } or try-finally should be used.

    4. You’re using XmlTextWriter. I assume this is at least .NET 2.0, so you should be using XmlWriter.Create() instead.

  56. My Version should be

    const String myConstStr = "CurrentFile";

           const String stylesheet = "..\..\XSLTFile1.xslt";        

           XslCompiledTransform xslt = null;

           XPathDocument doc = null;

           String filename = string.Empty;

           XmlTextWriter writer = null;

           for (int i = 0; i < 1000; i++)

           {

               xslt = new XslCompiledTransform();

               xslt.Load(stylesheet);

            filename = myConstStr + i.ToString() + ".xml";

               doc = new XPathDocument(filename);

               writer = new XmlTextWriter(filename, null);

               writer.Formatting = Formatting.Indented;            

               xslt.Transform(doc, null, writer);

               writer.Close();

               xslt = null;

               doc = null;

               filename = null;

               writer = null;

           }

  57. ren forever says:

    const String myConstStr = "CurrentFile{0}.xml";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    int i;

    XslCompiledTransform xslt = new XslCompiledTransform();

    xslt.Load(stylesheet);

    for(i = 0; i < 1000; i++){

        //Load the XML data file.

       String filename = String.Format( myConstStr, i.ToString());

       XPathDocument doc = new XPathDocument(filename);

       //Create an XmlTextWriter to write to the stream.

       XmlTextWriter writer = new XmlTextWriter(stream)

       writer.Formatting = Formatting.Indented;

       //Transform the file.

       xslt.Transform(doc, null, writer);

       writer.close();

    }

  58. Abd0 says:

    we should remove the XslTransform instantiation out of the for loop because we are creating a 1000 object just used one time and waiting to be collected by the garbage collector

  59. boen_robot says:

    The XSLT perhaps?

    And why define the writer before the XSLT transformation is completed? It would be one reason you use up some extra memory. Also, why indent it? Should it be human readable? Even if so, why not do it right before closing the XMLWriter (and after the XSLT transformation).

    Truth be told, I’m not an ASP coder. I just stumbled upon this, but seeing XSLT into the picture (my favorite language I’d say) I just couldn’t resist commenting.

  60. Myyzius says:

    Well I haven’t been playing around with anything but MOSS lately, which backdates me to VS2005 and .net 2.0.

    And I wouldn’t say the code is "wrong", I don’t generally correct wrongs, just try to efficiently use the available resources.

    What I would at least do differently, depending on what your .net version is:

    – I would make sure to reuse and clean the stream each time (you do have to recreate the writer on the stream since it’s forward only)

    – in relation to that, I fail to see why one would write 1000 xml objects transformed into the same stream? that would indeed use a lot of memory depending of course on your source xml

    So, I would perhaps recommend some other way of communicating the 1000 xml documents than through memory streams, seems a bit strange.

    – replace the string filename with a fileinfo object outside the loop of which you then just change the part beyond the path each time (no need to create a gazillion immutable strings in memory)

    – use said fileinfo to check if each source file exists and avoing nasty sproings

    – use fileinfo to check the existence of the transform file too, just to be nice (.exists aint that expensive)

    – xsltransform is obsolete in .net2 and thus -> later, replace with sys.xml.xsl.xslcompiledtransform

    – the new compiledtransform would be recommended to be loaded and compiled ONCE, outside the loop, for multiple optimized executions

    – the xml writer, I would recommend, should be used using the using statement. Alternatively flush and close / dispose of it each time.

    – the xmldocument which admittedly is the source, and seemingly loaded from a 1000 different source files, I am not sure without testing, but if one might save some overhead by having 1 of them and just reinitializing it to a new instance on each file loop, might save creating 999 of them? But since it’s read-only forward only, I would theorize that it might be immutable, and thus not usable in that fashion.

    – IF; and only IF, one experiences some really bad behaviour, call the GC once every 100 / 300 or so loops? I mean normally you would not need to, but say the computer running it is forced to low resources, then theoretically we could alleviate some memory pressure and reduce the GC’s need to do so itself, by hinting it to clean up stuff. The only difference you are likely to see would then be in the ram usage.

    Other than that, and the fact that I really don’t get WTH someone would write a thousand documents into a memory stream after each other for, I guess the code indeed would run. Not like the clappers no, but then transforming a 1000 objects using deprecated code into memory, using memory representations of each source document, well that would indeed use a lot of memory. So expected behavior. Not wrong. Kind of.

  61. james@skimming.net says:

    I’ve not been able to write a test program to prove anything, but I would start with the following ideas, ordered by which idea I suspect will yield the greater performance improvement.

    1. Instantiate and load the XSL Transformer object outside of the loop.

    2. Use the XslCompiledTransform object instead of the obsolete XslTransform object.

    3. Use this overload of the Transform method “Transform(string inputUri, XmlWriter results)”, which removes the need to load the document manually.

    4. Use StringBuilder to generate the filename of the source document.

  62. henkk says:

    Hmm. I did post an answer (sort of) yesterday. But it doesn’t seem to show up ?

    Short story: use XslCompiledTransform, create and load once, transform many times. The repeated load/transform on XslTransform will consume loads of mem.

    — Henkk

  63. Emre says:

    Just code;

    XslCompiledTransform xslt = new XslCompiledTransform();

               xslt.Load(@"d:XSLTFile.xsl");

               for (int i = 1; i <= 846; i++)

                   xslt.Transform(string.Format(@"d:xmlFilesPool ({0}).xml", i), string.Format(@"d:xmlFiles2Pool ({0}).html", i));

  64. Edwin says:

    I would change it like this :

    const String myConstStr = "CurrentFile";

    const String stylesheet = "..\..\XSLTFile1.xslt";

    int i = 0;

    XslTransform xslt = new XslTransform();

    xslt.Load(stylesheet);

    //Create an XmlTextWriter to write to the stream.        

    XmlTextWriter writer = new XmlTextWriter(stream);

    writer.Formatting = Formatting.Indented;

    for(i = 0; i < 1000; i++)

    {

       //Load the XML data file.

       String filename = myConstStr + i.ToString() + ".xml";

       XPathDocument doc = new XPathDocument(filename);

       //Transform the file.

       xslt.Transform(doc, null, writer);

    }

    writer.close();

    stream.Dispose();