0
\$\begingroup\$

I have a method that iterate over my table question in database and then will split a column commentaires and finnally put results in table propositions column commentaire so I have a code that read a large set of data DicCom 16400 rows and then lstProp with 7 rows so in total we have 116000 rows to modify.

Here is my code :

public static void UpdateDatabaseProCom()
{
    Dictionary<int, string[]> DicCom = new Dictionary<int, string[]>();
    List<string> listR = new List<string>
    {
        "R1",
        "R2",
        "R3",
        "R4",
        "R5",
        "R6",
        "R7"
    };

    var QueryListCom = from e in context.question
                       where e.Id_types == 5 && (e.Commentaires.StartsWith("/R") || e.Commentaires.StartsWith("/ R") || e.Commentaires.StartsWith("R1")) && listR.All(x => e.Commentaires.Contains(x))
                       select new { e.Id, e.Commentaires };


    foreach (var item in QueryListCom.ToDictionary(p => p.Id, p => p.Commentaires))
    {
        DicCom.Add(item.Key, item.Value.Split('/'));
    }

    foreach (var qstComSplit in DicCom.AsParallel())
    {
        var qst = context.question.FirstOrDefault(item => item.Id == qstComSplit.Key);
        var CommSplitted = qstComSplit.Value.Skip(1).ToArray();
        var lstProp = qst.proposition.ToArray();
        var counter = 0;

        for (int i = 0; i < lstProp.Count(); i++)
        {
            if (CommSplitted.Count() == 7 && lstProp.Count() == 7)
            {
                lstProp[i].Commentaires = CommSplitted[counter];
                context.SaveChanges();
                Console.WriteLine(qstComSplit.Key);
                counter++;
            }
        }
    }
}

My code take munch time to finsih, so I want to optimize it and I want to know how can I do that. I see that my code can be transformed to async await because it performs an expensive computation and have long-running operation so in this case we can talk about CPU-bound yes or no but with the running of my operation I don't need to yield the program to do another work so I have just a one task to do in my code ?

When I have this code it is a good idea to think to use Task ?

\$\endgroup\$
5
  • \$\begingroup\$ For starter, use Length instead of Count() on array objects such as lstProp and CommSplitted. Change that, and you'll improve your performance a lot. \$\endgroup\$
    – iSR5
    Apr 6, 2022 at 10:12
  • \$\begingroup\$ Have you done any kind of profiling? For example CodeTrack can help you to find the bottleneck of your solution. \$\endgroup\$ Apr 6, 2022 at 10:30
  • \$\begingroup\$ Is context a DbContext? If it is, you can't use AsParallel as DbContext isn't threadsafe. \$\endgroup\$
    – RobH
    Apr 6, 2022 at 10:37
  • \$\begingroup\$ @iSR5 Thank you it is more speed with Length \$\endgroup\$
    – Coder95
    Apr 6, 2022 at 10:44
  • \$\begingroup\$ @RobH context is an instantiation of bddEntities which is ( bddEntities : dbcontext ) \$\endgroup\$
    – Coder95
    Apr 6, 2022 at 10:46

1 Answer 1

2
\$\begingroup\$
  1. The most obvious improvement is to call SaveChanges() after the loops. This allows EF Core (I assume) to batch the updates for more performance.

  2. CommSplitted and lstProp are arrays (because of ToArray()). Use the array Length property instead of the LINQ extension method Count(). Count() will detect that this is an array and not loop the collection. Instead, it will call the Length property. But still, this involves a little overhead. For other collection types use the Count property (without parameter braces).

  3. By inspecting the inner loop more closely, it becomes apparent that the modifications will only be applied when the length of both arrays are equal to 7. Since the length of these arrays does not change during the execution of the loop, do this test before the loop.

    Or did you mean to do something different like basing the condition on a property of the array elements or just make sure both arrays have the same length with if (CommSplitted.Length == lstProp.Length)?

  4. The counter will always be equal to i. Therefore, you can use i as index in CommSplitted.

  5. The magic number 7 can be replaced by a constant. This allows you to give it a descriptive name. It also makes it easier and safer to change this number in future as you will have to do it only in a single place. You will be sure not to miss a place.

    Another magic number is Id_types 5. No idea what 5 stands for. A well named constant would clarify this.

  6. It makes no sense to add the questions to a dictionary that is never looked up by key (this is what a dictionary is for) just to loop over its elements and to add them to another dictionary which is also never looked up by key.

    If you intend to loop through a collection or to access the items by index, use an array (fixed length) or a List<T> (variable length). If you must store a pair of values, you can use a KeyValuePair<TKey, TValue> or a tuple as element type.

  7. Of what type is qst.proposition? I assume it to be a List<Proposition>. Calling .ToArray() has no apparent sense.

  8. In the foreach loop you are querying the very question by Id that you queried already in the first loop. This is an unnecessary round-trip to the database. Loop the questions returned form the database directly and do the split later.

  9. As @robH pointed out in a comment, do not use AsParallel().

  10. The length of listR is constant. Make it an array.

public static void UpdateDatabaseProCom()
{
    const int RequiredLength = 7;

    string[] listR = new[] { "R1", "R2", "R3", "R4", "R5", "R6", "R7" };

    var questions = from q in context.question
                            where q.Id_types == 5 && (
                                    q.Commentaires.StartsWith("/R") ||
                                    q.Commentaires.StartsWith("/ R") ||
                                    q.Commentaires.StartsWith("R1")
                                ) && listR.All(x => q.Commentaires.Contains(x))
                            select q;

    foreach (var question in questions) {
        string[] comments = question.Commentaires.Split('/').Skip(1).ToArray();
        if (comments.Length == RequiredLength && 
            question.proposition.Count == RequiredLength) {

            for (int i = 0; i < RequiredLength; i++) {
                question.proposition[i].Commentaires = comments[i];
                Console.WriteLine(question.Id);
            }
        }
    }
    context.SaveChanges();
}

I am not sure whether proposition is a list or an array. You may have to replace Count by Length in the latter case.

Note: EF Core will automatically split the updates done in the database by SaveChanges in reasonable sized batches. See Efficient Updating.

In case you need to know the number of changes, SaveChanges returns the number of database updates. int count = context.SaveChanges();

Avoid calling ToList, ToArray or ToDictionary on IQueryable<T> (resulting from querying a database) and on IEnumerable<T> if possible. This avoids unnecessary memory allocations. The items are being retrieved from the source as you are looping the result set without storing it in a collection.

In .NET Core and with C# 8.0+ you can replace .Skip(1).ToArray() with a range operator:

string[] comments = question.Commentaires.Split('/')[1..];

This operation is I/O (database) bound, not CPU bound. There are no complex calculations. Database operations require disk accesses measured in milliseconds (ms). CPU operations are measured in nanoseconds (ns). There is a factor of one million between ns and ms! Therefore, running parallel tasks will not help. Async/await does not speed up things, it only keeps the UI responsive while the operation is running.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.