Why an additional async operation is making my code faster than when the operation is not taking place at all?

Question

I'm working on a SMS-based game (Value Added Service), in which a question must be sent to each subscriber on a daily basis. There are over 500,000 subscribers and therefore performance is a key factor. Since each subscriber can be a difference state of the competition with different variables, database must be queried separately for each subscriber before sending a text message. To achieve the best performance I'm using .Net Task Parallel Library (TPL) to spawn parallel threadpool threads and do as much async operations as possible in each thread to finally send texts asap.

Before describing the actual problem there are some more information necessary to give about the code.

At first there was no async operation in the code. I just scheduled some 500,000 tasks with the default task scheduler into the Threadpool and each task would work through the routines, blocking on all EF (Entity Framework) queries and sequentially finishing its job. It was good, but not fast enough. Then I changed all EF queries to Async, the outcome was superb in speed but there has been so many deadlocks and timeouts in SQL server that about a third of the subscribers never received a text! After trying different solutions, I decided not to do too many Async Database operations while I have over 500,000 tasks running on a 24 core server (with at least 24 concurrent threadpool threads)! I rolled back all the changes (the Asycn ones) expect for one web service call in each task which remained Async.

Now the weird case:

In my code, I have a boolean variable named "isCrossSellActive". When the variable is set some more DB operations take place and an asycn webservice call will happen on which the thread awaits. When this variable is false, none of these operations will happen including the async webservice call. Awkwardly when the variable is set the code runs so much faster than when it's not! It seems like for some reason the awaited async code (the cooperative thread) is making the code faster.

Here is the code:

public async Task AutoSendMessages(...)
    {

        //Get list of subscriptions plus some initialization



        LimitedConcurrencyLevelTaskScheduler lcts = new LimitedConcurrencyLevelTaskScheduler(numberOfThreads);
        TaskFactory taskFactory = new TaskFactory(lcts);
        List<Task> tasks = new List<Task>();

        //....

        foreach (var sub in subscriptions)
        {
            AutoSendData data = new AutoSendData
            {
                ServiceId = serviceId,
                MSISDN = sub.subscriber,
                IsCrossSellActive = bolCrossSellHeader
            };

            tasks.Add(await taskFactory.StartNew(async (x) =>
            {
                await SendQuestion(x);
            }, data));
        }

        GC.Collect();

        try
        {
            Task.WaitAll(tasks.ToArray());
        }
        catch (AggregateException ae)
        {
            ae.Handle((ex) =>
            {
                _logRepo.LogException(1, "", ex);
                return true;
            });
        }

        await _autoSendRepo.SetAutoSendingStatusEnd(statusId);
    }

public async Task SendQuestion(object data)
    {
        //extract variables from input parameter

        try
        {
            if (isCrossSellActive)
            {
                int pieceCount = subscriptionRepo.GetSubscriberCarPieces(curSubscription.service, curSubscription.subscriber).Count(c => c.isConfirmed);

                foreach (var rule in csRules)
                {
                    if (rule.Applies) 
                    {
                        if (await HttpClientHelper.GetJsonAsync<bool>(url, rule.TargetServiceBaseAddress))
                        {
                            int noOfAddedPieces = SomeCalculations();

                            if (noOfAddedPieces > 0)
                            {
                            crossSellRepo.SetPromissedPieces(curSubscription.subscriber, curSubscription.service,
                                    rule.TargetShortCode, noOfAddedPieces, 0, rule.ExpirationLimitDays);
                            }
                        }
                    }
                }
            }
// The rest of the code. (Some db CRUD)
await SmsClient.SendSoapMessage(subscriber, smsBody);
        }
catch (Exception ex){//...}
}

Answer 1

Ok, thanks to @usr and the clue he gave me, the problem is finally solved! His comment drew my attention to the awaited taskFactory.StartNew(...) line which sequentially adds new tasks to the "tasks" list which is then awaited on by Task.WaitAll(tasks);

At first I removed the await keyword before the taskFactory.StartNew() and it led the code towards a horrible state of malfunction! I then returned the await keyword to before taskFactory.StartNew() and debugged the code using breakpoints and amazingly saw that the threads are ran one after another and sequentially before the first thread reaches the first await inside the "SendQuestion" routine. When the "isCrossSellActive" flag was set despite the more jobs a thread should do the first await keyword is reached earlier thus enabling the next scheduled task to run. But when its not set the only await keyword is the last line of the routine so its most likely to run sequentially to the end.

usr's suggestion to remove the await keyword in the for loop seemed to be correct but the problem was the Task.WaitAll() line would wait on the wrong list of Task<Task<void>> instead of Task<void>. I finally used Task.Run instead of TaskFactory.StartNew and everything changed. Now the service is working well. The final code inside the for loop is:

tasks.Add(Task.Run(async () =>
            {
                await SendQuestion(data);
            }));

and the problem was solved. Thank you all.

P.S. Read this article on Task.Run and why TaskFactory.StartNew is dangerous: http://blog.stephencleary.com/2013/08/startnew-is-dangerous.html

Answer 2

It's extremly hard to tell unless you add some profiling that tell you which code is taking longer now.

Without seeing more numbers my best guess would be that the SMS service doesn't like when you send too many requests in a short time and chokes. When you add the extra DB calls the extra delay make the sms service work better.

A few other small details:

await Task.WhenAll is usually a bit better than Task.WaitAll. WaitAll means the thread will sit around waiting. Making a deadlock slightly more likely.

Instead of:

        tasks.Add(await taskFactory.StartNew(async (x) =>
        {
            await SendQuestion(x);
        }, data));

You should be able to do

        tasks.Add(SendQuestion(data));