locked
How can I make this code faster? RRS feed

  • Question

  • Hi

    I have MANY events that do the following every 500 ms. I have a feeling that the way I am doing could be improved, but I don't know how, so I really need your help.

    Here is my code, can you help me make it faster or more efficient..?

    Thanks in advance

    static void timer1(object sender, System.Timers.ElapsedEventArgs e)
            {
                while (arrayTmp == null)
                {
                    Console.WriteLine("array = null");
                }
    
                double[] array1 = new double[111];
                Array.Copy(arrayTmp, 50, array1, 1, 109);
    
                List<double> list_obj = new List<double>();
                List<double> list_id = new List<double>();
    
                double id;
                for (int i = 0; i <= array1.Length; i++)
                {
                    if (array1[i] < flagD)
                    {
                        id = deg2rad((Convert.ToDouble(i + 50)) / 2);
                        list_obj.Add(array1[i]);
                        list_id.Add(id);
                    }
                }
                double dif_max_prev = 0;
                double dif_max_cur = 0;
                int flag = 0;
                double distance = 0;
                if (list_id.Count > 1)
                {
                    for (int i = 0; i < list_obj.Count - 1; i++)
                    {
                        dif_max_cur = Math.Abs(list_id[i + 1] - list_id[i]);
    
                        if (dif_max_cur > dif_max_prev)
                        {
                            dif_max_prev = dif_max_cur;
                            flag = i;
                        }
                    }
                    distance = pythagorean(list_id[flag], flagD, list_id[flag + 1], flagD);
                    if (distance > minimumAllowance)
                    {
                        distance = array1.Average();
                    }
                    else
                    {
                        distance = small;
                    }
                }
                else
                {
                    distance = big;
                }
            }


    • Edited by TuBong Friday, August 3, 2012 3:25 PM
    Friday, August 3, 2012 3:24 PM

Answers

  • Hello,

     

    To make it run faster get rid of redudant loops. There is 3 loop you have

    1.ArrayCopy : instead of this user orijinal array

    2. Fill List

    3. Find max diff

    Reduce to 1 loop like

        double sum = 0, prevval = double.NaN, val1, val2, maxdiff = double.MinValue, distance;
                //Array.Copy from 50 of temparray to 1 of array 1 so start with 49 end 161. value 49 and 161 zero
                for (int i = 49; i < 161; i++)
                {
                    //First and last value is zero other starts from index 50
                    double v = i == 49 || i == 160 ? 0 : arrayTemp[i];
                    if (v < flagD)
                    {
                        double id = deg2rad((Convert.ToDouble(i + 1)) / 2);
                        //Find max diff in loop
                        if (i > 49)
                        {
                            var dif = Math.Abs(id - prevval);
                            if (dif > maxdiff)
                            {
                                val1 = id; val2 = prevval;
                                maxdiff = dif;
                            };
                        }
                        //sum for calculating avarage
                        sum += v;
                      
                        //if this list is only for avarage remove this list
                        list_obj.Add(v);
    
                        
                        list_id.Add(id);
                        prevval = id;
                    }
                }
                if (list_id.Count > 1)
                {
                    distance = pythagorean(val1, flagD, val2, flagD);
                    if (distance > minimumAllowance)
                    {
                        //Calculate avarage
                        distance = sum / 111;
                    }
                    else
                    {
                        distance = small;
                    }
                }
                else
                {
                    distance = big;
                }

    deg2radian methods return same values  every time can be cacheable. So create  a static array for degtoradian values  and fill it the use. Dont user math libarary on each timer call..

    last You have loop of 111 iteration no need any paralell.

     

    • Edited by MDinc Monday, August 6, 2012 2:46 PM code update
    • Proposed as answer by Lisa Zhu Friday, August 10, 2012 4:00 AM
    • Marked as answer by Lisa Zhu Tuesday, August 14, 2012 10:34 AM
    Monday, August 6, 2012 2:44 PM

All replies

  • When the number of elements is known, a list reduces performance compared to an array.
    Friday, August 3, 2012 4:06 PM
  • Hi

    The number is unknown, as the List only stores any elements in the array that is meets the check condition.

    Friday, August 3, 2012 4:13 PM
  • Hi,

    1. Change this line

    id = deg2rad((Convert.ToDouble(i + 50)) / 2);

    by this one

    id = (Convert.ToDouble(i + 50)) *0.008726646259971647

    2. What's inside your "pythagorean" function?

    If it's something like

    Pythagorean(x1,x2,y1,y2) = Math.Sqrt(Math.Pow(x1-x2,2.0)+Math.Pow(y1-y2,2.0)) then that's awfully slow.

    You should better change it by

    Pythagorean(x1,x2,y1,y2) = Math.Sqrt(x1*x1-2.0*x1*x2+x2*x2+y1*y1-2.0*y1*y2+y2*y2)

    Math.Pow is a bad choice for computing squares and cubes.

    It evaluates x^2 as exp(ln(2.0)*x), which is more CPU-intensive that running just x^2 = x*x


    Sebastian Sajaroff Senior DBA Pharmacies Jean Coutu

    Friday, August 3, 2012 4:14 PM
  • hi

    In Pythagorean i have this

            private static double pythagorean(double angle1, double d1, double angle2, double d2)
            {
                // calculate distance between two object on the sides of the SD path
                double teta = Math.Abs(angle1 - angle2);
    
                double c = Math.Sqrt(Math.Pow(d1, 2) + Math.Pow(d2, 2) - (2 * d1 * d2) * Math.Cos(teta));
                return c;
            }

    Friday, August 3, 2012 4:17 PM
  • Start changing the following lines :

    double teta = angle1 - angle2;

    double c = Math.Sqrt(d1 * d1 + d2 * d2 - (2 * d1 * d2) * Math.Cos(teta));

     

    Why these changes?

    First of all, cos(teta) = cos(-teta), so there's no need to compute teta's absolute value //

    Second, Math.Pow() is highly inefficient to calculate real squares (uses logarithms and exponentials behind curtains), it's better to multiply number by itself.

     


    Sebastian Sajaroff Senior DBA Pharmacies Jean Coutu

    Friday, August 3, 2012 4:24 PM
  • I have projects like this one my self .. the only way to make it run faster and less work on the compiler is .. in your forloops when the condition is met, change the loop length to the max-length after

    list_id.Add(id);

    Right now you have it so that if the condition is met on the first loop , it will still keep looping threw x amont of times and then give the results, once the condition is met you should get out of the loop....

    I hope this helps..

    Friday, August 3, 2012 4:26 PM
  • I have projects like this one my self .. the only way to make it run faster and less work on the compiler is .. in your forloops when the condition is met, change the loop length to the max-length after

    list_id.Add(id);

    Right now you have it so that if the condition is met on the first loop , it will still keep looping threw x amont of times and then give the results, once the condition is met you should get out of the loop....

    I hope this helps..

    Hi

    Can i don't quite understand your suggestion. Can you elaborate more, or post the quick fix for the above code if possible?

    Friday, August 3, 2012 4:34 PM
  • Start changing the following lines :

    double teta = angle1 - angle2;

    double c = Math.Sqrt(d1 * d1 + d2 * d2 - (2 * d1 * d2) * Math.Cos(teta));

     

    Why these changes?

    First of all, cos(teta) = cos(-teta), so there's no need to compute teta's absolute value //

    Second, Math.Pow() is highly inefficient to calculate real squares (uses logarithms and exponentials behind curtains), it's better to multiply number by itself.

     


    Sebastian Sajaroff Senior DBA Pharmacies Jean Coutu

    Hi

    I'll do what you suggest. sounds a lot of sense to me too.

    Friday, August 3, 2012 4:34 PM
  • Hi

    The number is unknown, as the List only stores any elements in the array that is meets the check condition.

    It seems that the number is known to be less than 111.  The only unknown is the count of valid elements in the array.
    Friday, August 3, 2012 4:44 PM
  • Hi

    The number is unknown, as the List only stores any elements in the array that is meets the check condition.

    It seems that the number is known to be less than 111.  The only unknown is the count of valid elements in the array.

    Hi

    Ya Okay, I see what you means. I'll think a bout that. Thanks for suggestion.

    Friday, August 3, 2012 5:17 PM
  • Hi

    Can I use Parallel process anywhere in this code?

    Friday, August 3, 2012 5:18 PM
  • If you have many events doing this operation, they may already be saturating the CPU, so making each one parallel certainly isn't going to help.

    If these events are happening concurrently you many want to run this under the VS Concurrency Visualizer to see if there would even be room for more CPU usage.

    Also, unless these arrays are really big, making the code parallel probably wouldn't do much good because you'd just end up with lots of false cache swapping as all of the different tasks would be operating against the same data collections in the end.

    My last thought would be to instantiate the List objects to the size of the array.

    Friday, August 3, 2012 5:31 PM
  • If you have many events doing this operation, they may already be saturating the CPU, so making each one parallel certainly isn't going to help.

    If these events are happening concurrently you many want to run this under the VS Concurrency Visualizer to see if there would even be room for more CPU usage.

    Also, unless these arrays are really big, making the code parallel probably wouldn't do much good because you'd just end up with lots of false cache swapping as all of the different tasks would be operating against the same data collections in the end.

    My last thought would be to instantiate the List objects to the size of the array.

    Hi

    My array is less than 400 elements. And, these events (less than 10) are running concurrently - as all triggered by 1 timer @ 500 ms.

    My CPU is not saturated, it is at below 50% when running the program.

    I am really interest in knowing how to do concurrent. Can you give me a start on how to go around doing that?



    Besides, I initialized the list to 111.
    • Edited by TuBong Friday, August 3, 2012 5:46 PM
    Friday, August 3, 2012 5:38 PM
  • The first that you'd want to do would be to change either of your for loops into a Parallel.For loop. Given the size of the array, it's quite possible that the overhead of parallelizing this work is going to be greater than the time it takes to execute it. I still suspect you'll end up with a net loss.

    Since you have the events already going concurrently I suspect that you already are hitting processor cache misses. You can measure to see if you have a noticeable number of misses by using the Visual Studio sampling profiler and configuring it to track L2 Cache misses. You can read about that http://blogs.msdn.com/b/visualizeparallel/archive/2011/05/13/processor-cache-misses-are-reported-as-execution.aspx.

    Why are you having multiple threads executing the same code against the same arrayTmp?

    Friday, August 3, 2012 6:45 PM
  • The first that you'd want to do would be to change either of your for loops into a Parallel.For loop. Given the size of the array, it's quite possible that the overhead of parallelizing this work is going to be greater than the time it takes to execute it. I still suspect you'll end up with a net loss.

    Since you have the events already going concurrently I suspect that you already are hitting processor cache misses. You can measure to see if you have a noticeable number of misses by using the Visual Studio sampling profiler and configuring it to track L2 Cache misses. You can read about that http://blogs.msdn.com/b/visualizeparallel/archive/2011/05/13/processor-cache-misses-are-reported-as-execution.aspx.

    Why are you having multiple threads executing the same code against the same arrayTmp?

    Hi

    arrayTemp is one array, which got updated by a timer.

    so treating this array as the main array,

    i am slicing it into multiple pieces.

    so in those events, i am only working on a part of the main array.

    Friday, August 3, 2012 7:03 PM
  • for (int i = 0; i <= array1.Length; i++)
         {
           if (array1[i] < flagD)
              {
                id = deg2rad((Convert.ToDouble(i + 50)) / 2);
                list_obj.Add(array1[i]);
                list_id.Add(id);

               change to max length here;

              }
         }

    once (array[i] ) condition is meet, set the array1.Length to maxlength this will end the loop.. then your not doing looping for nothing....

    Friday, August 3, 2012 7:09 PM
  • Better to just use a break statement to exit the loop early.
    Friday, August 3, 2012 8:54 PM
  • It depends on what that loop is used for or how it's used, but your right.... either or.....
    Friday, August 3, 2012 9:34 PM
  • I would replace this:
    while (arrayTmp == null)
    {
            Console.WriteLine("array = null");
    }

    with this:
    if (arrayTmp == null)
    {
            Console.WriteLine("array = null");
            return;
    }

    Monday, August 6, 2012 7:16 AM
  • Yep... I for got to look at that too..

    Anything that doesn't have to be read or used then escape it.....

    Later

    Monday, August 6, 2012 12:50 PM
  • Hello,

     

    To make it run faster get rid of redudant loops. There is 3 loop you have

    1.ArrayCopy : instead of this user orijinal array

    2. Fill List

    3. Find max diff

    Reduce to 1 loop like

        double sum = 0, prevval = double.NaN, val1, val2, maxdiff = double.MinValue, distance;
                //Array.Copy from 50 of temparray to 1 of array 1 so start with 49 end 161. value 49 and 161 zero
                for (int i = 49; i < 161; i++)
                {
                    //First and last value is zero other starts from index 50
                    double v = i == 49 || i == 160 ? 0 : arrayTemp[i];
                    if (v < flagD)
                    {
                        double id = deg2rad((Convert.ToDouble(i + 1)) / 2);
                        //Find max diff in loop
                        if (i > 49)
                        {
                            var dif = Math.Abs(id - prevval);
                            if (dif > maxdiff)
                            {
                                val1 = id; val2 = prevval;
                                maxdiff = dif;
                            };
                        }
                        //sum for calculating avarage
                        sum += v;
                      
                        //if this list is only for avarage remove this list
                        list_obj.Add(v);
    
                        
                        list_id.Add(id);
                        prevval = id;
                    }
                }
                if (list_id.Count > 1)
                {
                    distance = pythagorean(val1, flagD, val2, flagD);
                    if (distance > minimumAllowance)
                    {
                        //Calculate avarage
                        distance = sum / 111;
                    }
                    else
                    {
                        distance = small;
                    }
                }
                else
                {
                    distance = big;
                }

    deg2radian methods return same values  every time can be cacheable. So create  a static array for degtoradian values  and fill it the use. Dont user math libarary on each timer call..

    last You have loop of 111 iteration no need any paralell.

     

    • Edited by MDinc Monday, August 6, 2012 2:46 PM code update
    • Proposed as answer by Lisa Zhu Friday, August 10, 2012 4:00 AM
    • Marked as answer by Lisa Zhu Tuesday, August 14, 2012 10:34 AM
    Monday, August 6, 2012 2:44 PM