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

Artykuł
09/23/2008

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

Anonymous
September 22, 2008
PingBack from http://www.easycoded.com/pop-quiz-what%e2%80%99s-wrong-with-this-code-%e2%80%93-part-3/
Anonymous
September 22, 2008
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.
Anonymous
September 22, 2008

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.
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();

Anonymous
September 22, 2008
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.
Anonymous
September 22, 2008
Why would you want to transform an xml file a thousand times in the first place?!
Anonymous
September 22, 2008
XSLTransform/XmlTextWriter instantiation needs to be moved outside the loop.. a 1000 instances are being created
Anonymous
September 22, 2008
The comment has been removed
Anonymous
September 22, 2008
vb version: http://pastebin.com/m61c43504
Anonymous
September 22, 2008
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
Anonymous
September 22, 2008

The XslTransform shall be created and initialized (.Load) outside of the loop.
A compiled Transform shall be used

Anonymous
September 23, 2008
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.
Anonymous
September 23, 2008
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.
Anonymous
September 23, 2008
No need to load the xsl inside the loop, just initialize it outside the loop.
Anonymous
September 23, 2008
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; }
Anonymous
September 23, 2008
I would first suggest having the XLST load be outside the loop so you arent creating 1000 wasted instances
Anonymous
September 23, 2008
loading the same stylesheet 1000 times comes to mind. So does creating/closing the writer the same number of times.
Anonymous
September 23, 2008
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.
Anonymous
September 23, 2008
Surely there's no need to load a XslTransform stylesheet 1000 times??
Anonymous
September 23, 2008
It looks like there's no reason why the xslt object couldn't be declared outside the loop since it never changes.
Anonymous
September 23, 2008
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 ;)
Anonymous
September 23, 2008
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....
Anonymous
September 23, 2008
I'm going to go with: The document is being loaded for each iteration of the loop.
Anonymous
September 23, 2008
Move XslTransform xslt = new XslTransform(); xslt.Load(stylesheet); out of the 1000 loop... and not sure about the other!
Anonymous
September 23, 2008
The XSLTFile1.xslt is being loaded on every iteration of the loop. Load it once before the loop.
Anonymous
September 23, 2008
Taking a few shots at it:

Shouldn't you be .Dispose()ing the XmlTextWriter object when you're done with it?
You only need to declare the XslTransform object once (and .Load() it as well). That's all I got.

Anonymous
September 23, 2008
concat inside for not using StringBuilder?
Anonymous
September 23, 2008
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...
Anonymous
September 23, 2008
The comment has been removed
Anonymous
September 23, 2008
dont check that the file actually exists beforehand should be in try-catch blocks to catch errors
Anonymous
September 23, 2008
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 :) } } }
Anonymous
September 23, 2008

First two lines in the for loop...the transform is loaded on every iteration. Move it before the loop. Why on earth would you.... :)
Same case with the XMLTextWriter. We're creating and closing it on every iteration. No need.

Anonymous
September 23, 2008
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
Anonymous
September 23, 2008
From first glance of the code snippet, I found the following:

Instantiating and loading the XSLT can be done before the loop.
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)
We can use regurlar XMLDocument instead of XPathDocument to load the data files within the loop

Anonymous
September 23, 2008
I think I spot a few things:

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.
The document loaded into the XslTransform instance should only be loaded once - in the example it is loaded 1000 times.
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?
The XmlTextWriter instance isn't being disposed. This might be consuming resources (i.e. memory) until the object is finalised by the garbage collector.

Anonymous
September 23, 2008
"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.
Anonymous
September 23, 2008

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).

Anonymous
September 23, 2008
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
Anonymous
September 23, 2008
Nice questions! I just discovered your blog and read through the old trivia questions. For this one, I'd say that:

The XSLT stylesheet (and the respective XslTransform) is static and should only be loaded once.
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).
XslTransform is outdated and XslCompiledTransform should be used instead.
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.

Anonymous
September 23, 2008
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.
Anonymous
September 23, 2008
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); } }

Anonymous
September 23, 2008
Dispose() not called on writer.
Anonymous
September 23, 2008
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?
Anonymous
September 23, 2008
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
Anonymous
September 23, 2008

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

Anonymous
September 23, 2008
After first glimpse, here is what I think:

move the lines: XslTransform xslt = new XslTransform(); xslt.Load(stylesheet); before the loop start.
Create filename using string.Format() or StringBuilder
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.

Anonymous
September 23, 2008

XslTransform xslt = new XslTransform(); xslt.Load(stylesheet); Give this outside "for" -> why 1000 times declare and load XSLT file

Anonymous
September 23, 2008
The declaration and loading part should be outside the loop XslTransform xslt = new XslTransform(); xslt.Load(stylesheet);
Anonymous
September 23, 2008
The comment has been removed
Anonymous
September 23, 2008
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.
Anonymous
September 23, 2008
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? :)
Anonymous
September 23, 2008
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...)
Anonymous
September 23, 2008
I can outline several problems

Relative path of stylesheet variable. The code can run under diferrent working folders. So it may result in strange behavior and sporadic FileNotFound exceptions
XslTransform xslt = new XslTransform(); xslt.Load(stylesheet); can be moved out of the for loop. This can positively affect performance.
didn't see the stream variable initialization.

Anonymous
September 23, 2008
The comment has been removed
Anonymous
September 23, 2008
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...
Anonymous
September 23, 2008
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();}
Anonymous
September 23, 2008
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();}
Anonymous
September 23, 2008

The XslTransform should be instantiated and loaded outside the loop.
Where is stream defined?
If xslt.Transform throws an exception, writer is not closed properly => using(...) { } or try-finally should be used.
You're using XmlTextWriter. I assume this is at least .NET 2.0, so you should be using XmlWriter.Create() instead.

Anonymous
September 23, 2008
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; }
Anonymous
September 23, 2008
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(); }
Anonymous
September 23, 2008
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
Anonymous
September 23, 2008
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.
Anonymous
September 23, 2008
The comment has been removed
Anonymous
September 23, 2008
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.

Instantiate and load the XSL Transformer object outside of the loop.
Use the XslCompiledTransform object instead of the obsolete XslTransform object.
Use this overload of the Transform method “Transform(string inputUri, XmlWriter results)”, which removes the need to load the document manually.
Use StringBuilder to generate the filename of the source document.

Anonymous
September 23, 2008
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
Anonymous
September 24, 2008
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));
Anonymous
September 24, 2008
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();

Udostępnij za pośrednictwem

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

Comments

Dodatkowe zasoby