How to optimize these nested loops for better performance?

Question

I need to optimize this code so it can execute faster, even if that means using more memory:

for (int t = 0; t < Clientes[0].ChildNodes.Count; t++)
{
 for (int i = 0; i < Clientes[0].ChildNodes.Item(t).ChildNodes.Count; i++)
 {
 if (Clientes[0].ChildNodes.Item(t).ChildNodes.Item(i).Name.Contains("Service"))
 {
 int a = 0;
 int.TryParse(Clientes[0].ChildNodes.Item(t).ChildNodes.Item(i).Name.ToUpper().Replace("SERVICE", ""), out a);
 if (a.Equals(Type))
 {
 Queues[t].Send(QueueMessage);
 }
 }
 }
}

I have decided to avoid accessing Clientes[0] and its ChildNodes.Item(t) over and over in each loop by keeping them in new variables like this:

XmlNodeList clientZeroChildNodes ;
int clientZeroChildCount ;
XmlNodeList clientZeroItemTchildNodes ;
int clientZeroItemTchildCount;
string clientZeroItemTchildIname;
clientZeroChildNodes = Clientes[0].ChildNodes;
clientZeroChildCount = clientZeroChildNodes.Count;
for (int t = 0 ; t < clientZeroChildCount ; t++)
{
 clientZeroItemTchildNodes = clientZeroChildNodes.Item(t).ChildNodes;
 clientZeroItemTchildCount = clientZeroItemTchildNodes.Count;
 for (int i = 0; i < clientZeroItemTchildCount; i++)
 {
 clientZeroItemTchildIname = clientZeroItemTchildNodes.Item(i).Name;
 if (clientZeroItemTchildIname.Contains("Service"))
 {
 int a = 0;
 int.TryParse(clientZeroItemTchildIname.ToUpper().Replace("SERVICE", ""), out a);
 if (a.Equals(Type))
 {
 try
 {
 Queues[t].Send(QueueMessage);
 }
 catch (MessageQueueException mqe)
 {
 //log error about mqe
 }
 }
 }
 }
}

As you can see, the second for loop only takes certain items of the ChildNodes based on its name. I have tried to use LINQ so that the second for only iterates the list of ChildNodes that match both of the if conditions that are inside the loop but have not been able to do it.

Can you show me how would you improve the speed of the original code focusing on an efficient way to improve the loops and nested ifs? I am not trying to determine which line of code is taking longer and the performance of the Queues is not relevant for me.

• svick: "let me know if you think that my approach will improve the speed" You need to measure that yourself. Guessing about speed is night impossible.
• svick: Also, are you sure this is the code that takes the most time in your application? Which part causes slowdown (a profiler will tell you that)? Isn't it Queues[t].Send(QueueMessage);?
• Mauricio Gracia Gutierrez: so if I post this question on regular stackoverflow is OFF-TOPIC because is not a "programming question/problem" and If I post it nobody helps me ? I am sure that accesing and arrays of arrays each time on a for loop is less eficcient I am just asking for other ways people will improve this
• Mauricio Gracia Gutierrez: the title clearly says that I want to improve the NESTED LOOPS not really related to the Queues
• Mauricio Gracia Gutierrez: @Malachi or even better write it from scratch keeping the functionality but improving readability and performance

Answer 1

it looks like you are doing a lot with making all sorts of variables that are really confusing to look at, maybe I am simple minded and that is why I can't figure it all out.

not really sure what you are doing with the t but I changed it to i

 XmlNodelist ClientNodes;
ClientNodes = Clientes[0].ChildNodes;
XmlNode ChildNode;
int i = 0;
foreach ChildNode in ClienteNodes
{
 i++
 XmlNodeList ClientChildNodes;
 XmlNode ClientChildNodeChild; //don't keep my name scheme
 foreach ClientChildNodeChild in ClientChildNodes
 {
 string clientZeroItemTchildIname; 
 clientZeroItemTchildIname = (ClientChildNodeChild.Item.Name).ToUpper();
 if ( ClientZeroItemTchildIname.Contains("SERVICE"))
 {
 int a = 0;
 int.TryParse(ClientZeroItemTchildIname.Replace("SERVICE",""),out a);
 if (a.Equals(Type))
 {
 try
 {
 Queues[i].Send(QueueMessage)
 }
 catch (MessageQueueException mqe)
 {
 //log Error about mqe
 }
 }
 }
 }
}

not sure that I made it any faster, but this is way more readable.

there is one less Variable in my code than there is in your 'optimized' code.

Which is faster, foreach or a for loop?

Here is an answer to that question, where someone actually went through testing for loops against foreach loops. and it looks like the foreach is faster in some instances.

Answer to Performance difference for control structures 'for' and 'foreach' in C# (2009)

I also found a page where someone claims that a foreach loop takes longer and is generally good for collections, but then he recommends against it anyway.

Code Project Article "FOREACH Vs. FOR (C#)" (2004)

Threw that one in there in case you thought that I was biased. I still think the foreach would be better for readability, but I don't know what your code is doing so I don't know what is going to be more efficient for you. Readable/maintainable vs. faster? maybe. not sure though.

Something else to think about

Take the Send out of the Loop.

keep track of which Queues that you want to send in a variable outside of the loop and then once you are finished looping then send them all at the same time, outside of the loop.

Answer 2

I would highly suggest profiling your code to see where the bottleneck lies.

That said, one thing to note (aside from what has already been mentioned) is that XmlNodeList.Item(int) is typically \$O(n)\$ ^*. As a result, your loops are \$O(n^2)\$.

If your elements have a large number of children (100+), you may want to switch to foreach loops, which will be \$O(n)\$:

foreach (XmlNode node in foo.ChildNodes)
{
 // Do stuff.
}

---

^* The IL for System.Xml.dll reveals that XmlNode.ChildNodes returns type XmlChildNodes, and XmlChildNodes.Item(int) is \$O(n)\$.

Answer 3

Before spending too much time optimizing the if's and for's, check if it will make any difference at all.

var queueIndexes = new List<int>();

In your inner loop:

//Queues[t].Send(QueueMessage)
queueIndexes.Add(t);

Then time this code:

foreach(var t in queueIndexes) {
 Queues[t].Send(QueueMessage);
}

The above for loop is as fast as you can get. How does this compare with the original? This will tell you if it's worth optimizing.

As a side note, I find it odd that you only use "t" from the outer loop, and don't do anything with the information from the inner loop, other than deciding whether to call Send. Is that intentional?

• Mauricio Gracia Gutierrez: I also find it odd, but what this two loops are acomplishing is to replicate one message in many different queues as many times as needed (consider one message that arrives and needs to be notified to different clients)

Answer 4

• Make Queues[t].Send(QueueMessage); asynchronous if it is not. This will be the best means to speed up your looping. Then have it handle the exception rather than this loop.

• If you're not expecting exceptions when calling Send, then having the Try/Catch does not slow you down. However, if you're catching multiple exceptions while processing, it is faster to perform sanity checks on the data before processing.

• You're doing a case-sensitive check on "Service" in the IF, but then you're doing a case-insensitive replace via .ToUpper() and "SERVICE". Remove the .ToUpper() if it is in fact case-sensitive. Otherwise that is a bug and should be checked in both places - for which you'd make a variable to house the result of .ToUpper() rather than calling it twice.

• Mauricio Gracia Gutierrez: thanks for your feedback, and does using intermediate variables helps in order to over the hierachy of objects again and again ?
• bland: @MauricioGracia Implement DanLyons' answer. He noted the speed in enumerating the list rather than accessing items by index, whereas I focused on the Queues. Index by integer is faster than by string (Name) for nodes and attributes, but I think he is right in general to move to the foreach and if you're here trying to speed up then either Async the Send or you have well over 100 items to iterate.
• svick: Just making something asynchronous won't magically improve performance.
• Mauricio Gracia Gutierrez: @bland the send on a queue can fail if the the contents of the message are checked. about the ToUpper it was a typo y int the code I provided. and the main idea is about how to improve the loops not so much the queue performance itself