-8

How to increase performance (it takes about 10 seconds) of this foreach loop

foreach (var item in pList)
{
    var foundedItem = source.FirstOrDefault(x => x.LongRecordId == item.LongRecordId);
    if (foundedItem != null && !selectionList.Contains(foundedItem))
    {
        foundedItem.Readonly = pReadonly;
        selectionList.Add(foundedItem);
    }
}
Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
Erhan Uslu
  • 15
  • 4
  • Increasing performance is not an easy task. But maybe you could try to use the actual class for your `item` instead of `var`. – Filnor Sep 15 '17 at 07:48
  • use for loop instead. – SᴇM Sep 15 '17 at 07:49
  • 1
    The problem might be your `selectionList.Contains(foundedItem)`. I don't know how complex your item is and how many items you store in that list. But you might wanna try a `Dictionary` instead. – Ferhat Sayan Sep 15 '17 at 07:50
  • 1
    the bottleneck may be the LINQ on `source`. what is the typical relation of numbers of items in `pList` as compared to `source`? precomputing either into a dictionary can be one approach. do some profiling to really find out (supposing you mean the performance including the loop body code, not just that of the `pList` iterator, which you don't show) – Cee McSharpface Sep 15 '17 at 07:51
  • Exactly what I've said. – wp78de Sep 15 '17 at 08:15

2 Answers2

4

In case source as well as pList are long, you may try converting source into a Dictionary:

 // Provinding that all LongRecordId are distinct
 var dict = source
    .ToDictionary(item => item.LongRecordId, item => item);

 ...

 foreach (var item in pList) {
   MyRecord foundItem = null; //TODO: Put the right type here

   // Instead of O(N) - FirstOrDefault we have O(1) TryGetValue
   if (dict.TryGetValue(item.LongRecordId, out foundItem)) {
     foundItem.Readonly = pReadonly;
     selectionList.Add(foundItem);
   } 
 }

Edit: In case you want to ensure that selectionList contains distinct items only (see Ferhat Sayan's comment) try HashSet

 ...

 HashSet<MyRecord> selectedHash = new HashSet<MyRecord>(selectionList);

 foreach (var item in pList) {
   MyRecord foundItem null; //TODO: Put the right type here

   // Instead of O(N) - FirstOrDefault we have O(1) TryGetValue
   if (dict.TryGetValue(item.LongRecordId, out foundItem)) {
     foundItem.Readonly = pReadonly;
     selectedHash.Add(foundItem);
   } 
 }

 selectionList = selectedHash.ToList();
Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
  • This example is missing the check if that item is already in selectionList, which could be a bottleneck aswell. But yah, it should lead to the right direction. – Ferhat Sayan Sep 15 '17 at 08:08
  • @Ferhat Sayan: if we want to ensure that `selectionList` contains distinct items only we can try `HashSet` – Dmitry Bychenko Sep 15 '17 at 08:15
  • thank you it very increase performance. Old version List open 10 second now 1 second this very helpful for me – Erhan Uslu Sep 15 '17 at 08:28
2

I suggest to profile that piece of code.

I guess the most time consuming call is the FirstOrDefault-Extension Method.

To use for instead of foreach is unlikely to grant you much if any performance gains.

Most probably your problems stems from your context variables, maybe a very large source list?

Overall, this code section is maybe not the best place to look for performance improvements. If this is the only place, you should think about a redesign.

If you have very large lists and do not fear concurrency issues you could go the parallel or multi-processing route. parallel.foreach is a cheap way to do some tests.

The use of a Dictionary instead of a List should speedup things. If you go with a HashSet (to yield the benefit of uniqunes) be aware to implement a proper IEqualityComparer, or you could even lose performance when you add a huge number of items relying on the DefaultComparer.

wp78de
  • 18,207
  • 7
  • 43
  • 71