locked
foreach Loop Performance RRS feed

  • Question

  • Hi,

    I fetch the database to my code using SQL command and set in DataBase, I have foreach loop on this datatable it's take long time when the command return big number of the records.

    any idea to enhance the performance.

    Notes: - Language is C# and the application is web application.

    - I used for loop but that's same.

    foreach (DataRow r in table.Rows)
                        {
    events.Add(new RptEventDetails
    {
         DriverId = Converter.ToInt32(r["Driverid"]),
         VehicleId = Converter.ToInt32(r["VehicleId"]),
         DriverName = r["DriverName"].ToString(),
         VehicleName = r["VehicleName"].ToString(),
         EventCode = Converter.ToInt32(r["EventCode"]),
        EventName = r["EventName"].ToString(),
       EventId = Converter.ToInt32(r["EventId"]),
        StartDate = Converter.ToDateTime(r["EventStart"]) ?? DateTime.Now,
        EndDate = Converter.ToDateTime(r["EventEnd"]) ?? DateTime.Now

    });

    }

    Thanks in advance. 


    • Edited by AhmadMou Wednesday, August 8, 2012 1:45 PM
    Wednesday, August 8, 2012 1:40 PM

Answers

  • And to add to Fernando`s post, to improve performance, use Parse methods instead of ConvertTo:

    int.Parse(yourValue) datetime.Parse(yourValue)

    if you are sure there are those values. If you are not, better to use Convert, because in case (int.Parse()) will thrown an error, if you will try to convert string for example, when Convert.ToInt32() will only return 0.

    About why your method need a lot of time to return, there is not such big difference between using string or index to get/set column, like Fernando explained there is a difference, but still not so much. 

    It seems you have a lot of rows inside database, thats why it takes so long, thats why I`d recommend you to put all this code on a new thread, and maybe use some progressBar control, to show to the user that there is something going on in the background.


    Mitja

    Wednesday, August 8, 2012 4:14 PM
  • The most time consuming part is adding object to events collection. Instead of adding items one by one, use AddRange function to add objects in bulk,

    RptEventDetails[] eventDetails= new RptEventDetails[table.Rows.Count);
    for(int i = 0; i < table.Rows.Count; i++)
    {
       DataRow r = table.Rows[i];
       eventDetails[i] = new RptEventDetails
              {
                  DriverId = Convert.ToInt32(r["Driverid"]),
                  VehicleId = Convert.ToInt32(r["VehicleId"]),
                  DriverName = Convert.ToString(r["DriverName"]),
                  VehicleName = Convert.ToString(r["VehicleName"]),
                  EventCode = Convert.ToInt32(r["EventCode"]),
                  EventName = Convert.ToString(r["EventName"]),
                  EventId = Convert.ToInt32(r["EventId"]),
                  StartDate = r["EventStart"] == null?  DateTime.Now : Converter.ToDateTime(r["EventStart"]); 
                  EndDate = r["EventEnd"] == null ? DateTime.Now : Convert.ToDateTime(r["EventEnd"]; 
             };
    }
    events.AddRange(eventDetails);
    Try this code.

    Please mark this post as answer if it solved your problem. Happy Programming!

    Thursday, August 9, 2012 5:22 AM
  • I suggest you use index rather than column name string while reading a field. To maintain readability while accessing column you can use enum.

    I also suggest rather than using 'For each' loop, consider normal loop (for int i=0; i<table.Rows.count; i++) - should be useful when the collection is large.

    Thursday, August 9, 2012 9:14 AM

All replies

  • Check this link, have some informations that could clear your mind.
    LINQ Vs foreach

    Web Developer


    • Edited by Norkk Wednesday, August 8, 2012 1:47 PM
    Wednesday, August 8, 2012 1:47 PM
  • Hi AhmadMou;

    Accessing the columns of a DataRow object using the string name is great for readability and maintainability but this is done at the cost of performance. Each time you request to get or set the column by using the string name the system needs to look up the index of the item and then use that index to do the action needed. By using the index of the column you gain better performance at the cost of readability and maintainability. Try changing the string names and use the column index and see if it buys you any real performance.

      


    Fernando (MCSD)

    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful".

    Wednesday, August 8, 2012 2:29 PM
  • Depending on how many records you have you might use Parallel.ForEach or other asyncronous approach to read data rows from table and initialize RptEventDetails instances. Then again if you take this step you must remember that multiple threads can access the events.Add method at the same time. This might give you performance boost, but possibly with cost of more complex code.

    Wednesday, August 8, 2012 3:03 PM
  • And to add to Fernando`s post, to improve performance, use Parse methods instead of ConvertTo:

    int.Parse(yourValue) datetime.Parse(yourValue)

    if you are sure there are those values. If you are not, better to use Convert, because in case (int.Parse()) will thrown an error, if you will try to convert string for example, when Convert.ToInt32() will only return 0.

    About why your method need a lot of time to return, there is not such big difference between using string or index to get/set column, like Fernando explained there is a difference, but still not so much. 

    It seems you have a lot of rows inside database, thats why it takes so long, thats why I`d recommend you to put all this code on a new thread, and maybe use some progressBar control, to show to the user that there is something going on in the background.


    Mitja

    Wednesday, August 8, 2012 4:14 PM
  • The most time consuming part is adding object to events collection. Instead of adding items one by one, use AddRange function to add objects in bulk,

    RptEventDetails[] eventDetails= new RptEventDetails[table.Rows.Count);
    for(int i = 0; i < table.Rows.Count; i++)
    {
       DataRow r = table.Rows[i];
       eventDetails[i] = new RptEventDetails
              {
                  DriverId = Convert.ToInt32(r["Driverid"]),
                  VehicleId = Convert.ToInt32(r["VehicleId"]),
                  DriverName = Convert.ToString(r["DriverName"]),
                  VehicleName = Convert.ToString(r["VehicleName"]),
                  EventCode = Convert.ToInt32(r["EventCode"]),
                  EventName = Convert.ToString(r["EventName"]),
                  EventId = Convert.ToInt32(r["EventId"]),
                  StartDate = r["EventStart"] == null?  DateTime.Now : Converter.ToDateTime(r["EventStart"]); 
                  EndDate = r["EventEnd"] == null ? DateTime.Now : Convert.ToDateTime(r["EventEnd"]; 
             };
    }
    events.AddRange(eventDetails);
    Try this code.

    Please mark this post as answer if it solved your problem. Happy Programming!

    Thursday, August 9, 2012 5:22 AM
  • I suggest you use index rather than column name string while reading a field. To maintain readability while accessing column you can use enum.

    I also suggest rather than using 'For each' loop, consider normal loop (for int i=0; i<table.Rows.count; i++) - should be useful when the collection is large.

    Thursday, August 9, 2012 9:14 AM
  • I agree with Manu - classic loops seem faster than using an iterator. Find one of my posts that actually compares timing on foreach, for, and lambda, for looking for a character over 85K characters - the classic for was generally 15% faster

    .Net thingy maker type guy.

    • Proposed as answer by rachmann Friday, August 10, 2012 12:35 AM
    Friday, August 10, 2012 12:34 AM