Replace nested foreach to Increase performance

Question

I have the following code which take longer time to execute. Is there any alternative that I can replace below code with LINQ or any other way to increase the performance?

var usedLists = new HashSet<long>();

foreach (var test in Tests)
{
    var requiredLists = this.GetLists(test.test, selectedTag);

    foreach (var List in requiredLists)
    {   
        if (!usedLists.Contains(List.Id))
        {
            usedLists.Add(List.Id);
            var toRecipients = List.RecepientsTo;
            var ccRecipients = List.RecipientsCC;
            var bccRecipients = List.RecipientsBCC;
            var replyTo = new List<string>() { List.ReplyTo };
            var mailMode = isPreviewMode ? MailMode.Display : MailMode.Drafts;
            OutlookModel.Instance.CreateEmail(toRecipients, ccRecipients, bccRecipients, this.Draft, mailMode, replyTo);
        }
    }
}

I doubt that you'll get it faster with linq and I suspect that the `CreateEmail` function takes up all the time. Profile your code and see where the real bottleneck is. http://stackoverflow.com/questions/3927/what-are-some-good-net-profilers — Stormenet, May 20 '14 at 11:29
What about your `this.GetLists` method? Except these two methods there is nothing that is likely to consume that much in this code. Or you might send a grouped mail at once if that is not against what you need to do (because you are always sending the same draft) — mmg666, May 20 '14 at 11:56
It is not clear how many Tests there are and where they come from. If each GetLists call is a query to a DB, and there are a lot of lists, you could speed it up by querying the DB in one go before entering the loop. — MarkO, May 20 '14 at 11:57

Nathan Adams · Answer 1 · 2014-05-20T16:26:55.627

It might be able to done with the BackgroundWorker class or something similar. A very rough outline would be to wrap the create email arguments into a separate class, then asynchronously start a background task while the loop continues processing the list.

Edit The Task class might be of use too. This is assuming the CreateEmail function is the one that's taking up all the time. If it's the loop that's taking all the time then there's no point in going this route though.

Edit Looking at the algorithmic complexity, the GetLists() calls run in O(n) time, but the CreateEmail() calls run in O(n^2) time, so - all other things being equal - the code block containing the CreateEmail() call would be a better candidate to optimise first.

score 1 · Answer 2 · answered May 20 '14 at 12:02

Assuming the result from GetList is large/slow enough in comparison to CreateEmail call, skipping your contains-add hashset comparison will speed things up. Try a call to Distinct([Your ListComparer]) or GroupBy():

var requiredLists = 
    Tests.SelectMany(test => this.GetLists(test.test, selectedTag))
         .Distinct([Your ListComparer]);
// or 
var requiredLists = 
    Tests.SelectMany(test => this.GetLists(test.test, selectedTag))
         .GroupBy(x => x.Id).SelectMany(x => x.First());

foreach (var List in requiredLists)
{   
    // (...)
    OutlookModel.Instance.CreateEmail(toRecipients, ccRecipients, bccRecipients, this.Draft, mailMode, replyTo);
}

You could also try PLINQ to speed up things but I think you might run into trouble with the CreateEmail call, as it's probably a COM object, so multithreading will be a hassle. If GetList is slow enough to be worth the multithread overhead, you may experiment with it in the first call when creating requiredLists.

score 1 · Accepted Answer · answered May 20 '14 at 13:19

What do you actually do? Looking at

foreach (var test in Tests)
{
    var requiredLists = this.GetLists(test.test, selectedTag);
    foreach (var List in requiredLists)
    {   
        if (!usedLists.Contains(List.Id))

it looks to me that you try to get unique "List"s (with all those "var" I cannot tell the actual type). So you actually could replace that with a new function (to be written by you) and do

var uniqueLists = this.GetUniqueLists(Tests, selectedTag);

and finally call

this.Sendmails(uniqueLists);

which would iterate thru that list and send the mails.

If that could speed up the code or not, depends severely on the underlying GetLists / GetUniqueLists functions.

But in any case, it would make a great progress to your code: it becomes readable and testable.

score 0 · Answer 4 · answered May 20 '14 at 11:42

Each foreach loop creates an Enumerator object, so using a regular for-loop might speed it up a little, but this might be a very minor improvement.
Like stormenet said, the CreateEmail function could be the bottleneck. You might want to make that asynchronous.

Replace nested foreach to Increase performance

4 Answers4