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.

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?

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.