4
\$\begingroup\$

I am writing an app that is supposed to simulate load on our web api and NServiceBus(particular) service. This is the first time I am writing an application like this. It appears to run ok, however it takes a few seconds before anything shows up in the console window. Is there a better way to do this?

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine("Press any key to start...");
        Console.ReadKey();
        Console.WriteLine();

        RunLoad(100);

        Console.WriteLine("Press any key to exit...");
        Console.ReadKey();
    }

    private static void RunLoad(int loadSize)
    {
        Parallel.For(0, loadSize, i =>
        {
            RunTasks();
        });
    }

    public static async Task RunTasks()
    {
        var tasks = new List<Task>
        {
            GetLoanByLoanId("7000002050"),
            GetEnvelopesForLoan("7000002077"),
            GetLoanDataByLoanId("7000002072")

        };

        await Task.WhenAll(tasks);
    }

    private static async Task GetLoanByLoanId(string loanNumber)
    {
        await Task.Run(() => {
            var svc = new WebApiService();
            var msg1 = svc.GetLoanByLoanId(loanNumber);
            if (string.IsNullOrWhiteSpace(msg1)) return;
            Console.WriteLine($"GL {loanNumber}: {msg1.Length}");
        });
    }

    private static async Task GetLoanDataByLoanId(string loanNumber)
    {
        await Task.Run(() => {
            var svc = new WebApiService();
            var msg1 = svc.GetLoanDataByLoanId(loanNumber);
            if (string.IsNullOrWhiteSpace(msg1)) return;
            Console.WriteLine($"GLD {loanNumber}: {msg1.Length}");
        });
    }

    private static async Task GetEnvelopesForLoan(string loanNumber)
    {
        await Task.Run(() => {
            var svc = new WebApiService();
            var msg1 = svc.GetEnvelopesForLoan(loanNumber);
            if (string.IsNullOrWhiteSpace(msg1)) return;
            Console.WriteLine($"GEFL {loanNumber}: {msg1.Length}");
        });
    }

}
}

I am trying to mimic the number to users hitting the system through the RunLoad method.

Also this is running in .Net Framework 4.8..

\$\endgroup\$
2
  • \$\begingroup\$ Do I understand correctly that your RunLoad method tries to mimic several clients? Are you concerning about ramp-up time? Do I understand correctly that your GetXYZ are independent? Does ordering of these API calls matter? \$\endgroup\$ Commented Jul 6, 2020 at 7:14
  • \$\begingroup\$ Yes RunLoad is supposed to mimic the number of clients.All the Get... are independent and no the order of the API calls do not matter... \$\endgroup\$ Commented Jul 6, 2020 at 11:12

2 Answers 2

3
\$\begingroup\$

There are several improvement areas. Let's review them one-by-one from bottom up:

GetXYZ

private static async Task GetXYZ(string parameter)
{
     await Task.Run(() => {
         var svc = new WebApiService();
         var msg1 = svc.GeXYZ(parameter);
         if (string.IsNullOrWhiteSpace(parameter)) return;
            Console.WriteLine($"XYZ {parameter}");
     });
}

A Task can represent either an asynchronous work or a parallel work. An async work can be considered as non-blocking I/O operation. Whereas the parallel work can be treated as (blocking) CPU operation. (It is oversimplified but it works for us now)

In the above code the Task.Run tells to .NET runtime that this piece of code should run a dedicated thread (in reality this is not as simple as this, but let me simplify the model now). It means that the passed delegate should run a dedicated CPU core.

But inside your delegate you are making a blocking I/O operation. So you have created a delegate, you have moved that code to a dedicated core, which will block and do nothing until the network driver will finish the I/O operation.

You are just wasting a lot of valuable resources. A better alternative would look like this:

private static async Task GetXYZ(string parameter)
{
     var svc = new WebApiService();
     var msg1 = await svc.GeXYZAsync(parameter);
     if (string.IsNullOrWhiteSpace(parameter)) return;
        Console.WriteLine($"XYZ {parameter}");
}

Here you are making a non-blocking I/O operation. The network driver fires of a network call asynchronously and returns immediately. When it completes it will notify the ThreadPool. (Because here we are calling await against an asynchronous work that's why we are not blocking the caller Thread.)

In short: Network driver can do let's say 1000 concurrent network operations at the same time. While the CPU can perform as many operations in parallel as many cores it has.

RunTasks

(I guess you can find a better name for this.)

Here with the Task.WhenAll you are already asking the .NET runtime to run them in parallel. If they are asynchronous operations then the network driver will take care of them and try to run them in parallel. In case of parallel work CPU tries to schedule them on different cores but there is no guarantee that they will run in parallel. They might run only concurrently (with context-switches).

RunLoad

(I guess you can find a better name for this.)

Here with the Parallel.For you try to create as many blocking parallel task as many the loadSize value. If you have 100 Tasks for 8 CPU cores then it is inevitable to have context-switches. Again your current implementation is highly CPU-bound even though you try to perform massive I/O operations.

A better alternative:

private static async Task RunLoad(int loadSize)
{
    var clients = new List<Task>();
    for (int j = 0; j < loadSize; j++)
    {
        clients.Add(RunTasks());
    }

    await Task.WhenAll(clients);
}

In this way all your client's all requests might run in parallel if your network driver supports that many outgoing requests.

This WhenAll will be finished when all operation has finished. If you want to monitor them in "real-time" when any of them finished then you can rewrite your method in the following way:

static async Task Main(string[] args)
{
    Console.WriteLine("Press any key to start...");
    Console.ReadKey();
    Console.WriteLine();
  
    await foreach (var test in RunTests(10))
    {
        //NOOP
    }

    Console.WriteLine("Press any key to exit...");
    Console.ReadKey();
}


private static async IAsyncEnumerable<Task> RunTests(int loadSize)
{
    var requests = new List<Task>();
    for (int j = 0; j < loadSize; j++)
    {
        requests.Add(GetLoanByLoanId("7000002050"));
        requests.Add(GetEnvelopesForLoan("7000002077"));
        requests.Add(GetLoanDataByLoanId("7000002072"));
    }

    while (requests.Any())
    {
        var finishedRequest = await Task.WhenAny(requests);
        requests.Remove(finishedRequest);
        yield return finishedRequest;
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thank you, yeah naming is not my strong point... \$\endgroup\$ Commented Jul 6, 2020 at 13:18
  • \$\begingroup\$ In your alternative RunLoad you have a list of tasks (RunTasks) waiting on the WhenAll, but RunTasks has the same sort of flow waiting on whenall of Getxxx methods to complete. Is a good idea? \$\endgroup\$ Commented Jul 6, 2020 at 15:07
  • \$\begingroup\$ @Kixoka RunTasks waits to finish all requests from a given client. RunLoad waits to finish all clients' all requests. It depends how you want to manage the relationships between requests and clients. If there is no particular relationship (just a bunch of requests with different origin) then the the RunTests makes sense, because it does not wait for any group of tasks to finish. If one finishes then it yields that back. Does it make sense for you? \$\endgroup\$ Commented Jul 6, 2020 at 15:38
3
\$\begingroup\$

Overall, the code is quite clean and easy to understand. You did a good job of subdividing the work into small and easily digestible submethods.

I did notice a few other things though, which I'll list below.


Tasks vs threads

Right now, it seems you're using Task.WhenAll and Task.Run as an async wrapper to emulate different concurrent threads. That's not what asynchronous code is for. While asynchronous code often does use multiple cores/threads, that's not a guarantee. You can run asynchronous logic on a single-core machine with a single thread, where there's no possible way of executing logic concurrently.

Do I think that you're working on a single-core machine with a single thread? No. But the point remains that you're currently conflating two different concepts that have some but not complete overlap.

I'm not saying your way of doing it doesn't work right now, but to ensure that this code works on all machines I would suggest that you use threads for this, not tasks. That way, you enforce concurrent logic instead of hoping that it works out.

Then again, if this is a small short-lived test app that will be discarded in the near future, it doesn't quite matter (but then a code review wouldn't really matter either, I guess).

I would generally expect WebApiService to have async methods, which you could be using here. However, it's possible that WebApiService doesn't have async methods and that you are unable/unwilling to implement them on WebApiService, which would be fair enough since this test application isn't trying to redevelop the service implementation but rather test the existing one.

Constructor spam

Right now you're creating a new WebApiService() 300 times (100 loops, 3 tasks each). Is that by intention, or could this be done using a single service object?

I suspect (though I cannot guarantee) you can reuse the same object without issues here, which means you could shift that instantiation outside of the loops and either pass it as a parameter or use an static variable (since everything else is static anyway).

private static WebApiService _svc = new WebApiService();

private static async Task GetLoanByLoanId(string loanNumber)
{
    await Task.Run(() => {
        var msg1 = _svc.GetLoanByLoanId(loanNumber);
        if (string.IsNullOrWhiteSpace(msg1)) return;
        Console.WriteLine($"GL {loanNumber}: {msg1.Length}");
    });
}

The same goes for the other two tasks. If you instead wish to pass it as a method parameter, simply instantiate it in Main() and then thread it through your method calls all the way down to the GetLoanByLoanId method (and the other two tasks).

Since everything here is static anyway, I'd use a static class field here because it's simpler and doesn't really cause any issues in your already exclusively static code.

If return

This is quite unidiomatic:

if (string.IsNullOrWhiteSpace(msg1)) return;
Console.WriteLine($"GEFL {loanNumber}: {msg1.Length}");
// end of void method

You could simply invert the if instead of returning one line early:

if(!String.IsNullOrWhiteSpace(msg1))
    Console.WriteLine($"GEFL {loanNumber}: {msg1.Length}");

This way, you don't need an eary return. Early returns have their uses when they can skip a whole lot of performance-draining steps, but when that if return is the last block of the method body, there's no futher logic (i.e. after the if) to skip.

Logging

It's weird that you're choosing to not log a message when an empty result was retrieved. You already don't quite seem to care about the received message since you're only logging its length, so I surmise that you're using these messages as for progress tracking.

I'm unsure why you're specifically not logging 0 length (or null) results. If the logging is to keep track of the progress, then those null-or-empty results are still progress and should be logged.

If you wrote this check to avoid null reference exceptions, there's better ways of avoiding those, e.g. null propagation:

Console.WriteLine($"GEFL {loanNumber}: {msg1?.Length}");

This way, you always get a message, regardless of what msg1 contains.

Reusable task logic

All three tasks have pretty much the exact same method body, except for two small differences:

private static async Task AnyTask(string loanNumber)
{
    await Task.Run(() => {
        var svc = new WebApiService();
        var msg1 = svc.THISMETHODISDIFFERENT(loanNumber);                      // <- here
        if (string.IsNullOrWhiteSpace(msg1)) return;
        Console.WriteLine($"THISNAMEISDIFFERENT {loanNumber}: {msg1.Length}"); // <- here
    });
}

But each of these methods you use always follows the same string MyMethod(string) pattern, and the log message is really just an arbitrary string value. You can therefore easily abstract this method call in a way that the rest of the task body can be reused:

private static async Task GetLoanByLoanId(string loanNumber)
{
    PerformJob($"GL {loanNumber}", svc => svc.GetLoanByLoanId(loanNumber));
}

private static async Task GetLoanDataByLoanId(string loanNumber)
{
    PerformJob($"GLD {loanNumber}", svc => svc.GetLoanDataByLoanId(loanNumber));
}

private static async Task GetEnvelopesForLoan(string loanNumber)
{
    PerformJob($"GEFL {loanNumber}", svc => svc.GetEnvelopesForLoan(loanNumber));
}

private static async Task PerformJob(string jobName, Func<WebApiService, string> getMessage)
{
    return Task.Run(() => {
        var svc = new WebApiService();
        var msg1 = getMessage(svc);
        if (string.IsNullOrWhiteSpace(msg1)) return;            
        Console.WriteLine($"{jobName}: {msg1.Length}");            
    });            
}

There are two things to remark here:

  • The three job methods have become so trivial that you could arguably remove them and just call PerformJob("xxx", svc => svc.xxx(loanNumber)); directly from the calling code. I think this is a subjective call whether you prefer to wrap it nicely or would rather avoid one liner methods.
  • If you follow the improvements I suggested in earlier points, the method body of PerformJob becomes trivial as well. But I wouldn't suggest removing this method even though it's trivial, since that would force you to copy/paste the log message format all over the place.
private static async Task PerformJob(string jobName, Func<WebApiService, string> getMessage)
{
    // Verbosely:
    var msg = getMessage(svc);
    Console.WriteLine($"{jobName}: {msg?.Length}");

    // Or as a one-liner:
    Console.WriteLine($"{jobName}: {getMessage(svc)?.Length}");          
}
\$\endgroup\$
2
  • \$\begingroup\$ Ultimately I am trying to simulate greater than 100+ users hitting the webAPI at once to evaluate scalability and to pin point bottlenecks not only with the webAPI but the supporting Particular/NServiceBus service that handles queue routing. I have logging set up on the API and Service to spit out any contentions etc.... \$\endgroup\$ Commented Jul 6, 2020 at 13:08
  • \$\begingroup\$ Also thank you for the review. Very much appreciated! \$\endgroup\$ Commented Jul 6, 2020 at 14:09

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.