Would this be a proper use of threading?

Question

After reading various MSDN articles, tutorials, and posts on here I came up with a design for a windows service that I wanted to make sure my threading strategy is proper and won't lead to memory, cpu, database issues.

Here is the basic flow.

MAIN THREAD

1. Collect list of service to be loaded.
2. Loads each service.

SERVICE THREAD (started by timer event on step 3)

1. Start Service
2. Setup Timer - Setup so there is no overlapping, only 1 processing call can be made.
3. Processing method
   • Makes database call to get information which is a list of Commands.
   • Gathers a collection of Handlers.
   • Does a Parallel.ForEach() over the list of Commands.
   • Immediately does a normal foreach() over the Handlers

COMMAND THREADS

1. Command does a normal foreach() over the Handlers passing in the Command.
2. Each Handler executes code that may CRUD the database or reach out to various other services.
   • Due to the Parallel.ForEach() I am spinning up 1 new thread per Command.
   • There could be 1000+
   • Since each Command has multiple Handlers these Handlers will run on the same thread as the Command.
   • This is to prevent database clashes since each Handler will be working with the same set of data.
   • This is to make each Command wait on its own Handler threads.

WAIT POINTS

1. Service waits for all Commands to finish executing before firing another round of round of commands.
2. Commands wait for all Handlers to to finish.
3. Handlers wait for one to finish before moving onto the next.

PERFORMANCE

• 3000x FOREACH Console.WriteLine() avg. 160ms
• 3000x FORALL Console.WriteLine() avg. 275ms
• 3000x FOREACH DatabaseCallAsync() avg. 1200ms
• 3000x FORALL DatabaseCallAsync() avg. 600ms

Interesting how the normal foreach was faster with just the WriteLine() but twice as slow with the database call. I still have concern about competing for resources with the other services.

QUESTIONS

1. Do you recognize any immediate issues with this design?
2. Do you have any suggestions that would make it more efficient with resources and speed?
3. I am noticing extremely high CPU usage on all cores when running just Console.WriteLines() in the actual execution point, should I be limiting the threads?
   • I noticed when I used a normal foreach() it still had pretty high cpu usage but had more resting points, I still need to put a counter in there to see how many calls are actually being made for each to see which is actually processing more but right now I am just concerned with the high usage.

Answer 1

• You do not need to use dedicated threads: a thread is an expensive object so it is generally better to use the thread pool since it will reuse threads to process your work items. Using a dedicated thread is a good idea if you need thread-local storage, thread identity, thread priorities, custom scheduling (picking work item from a stack or a priority queue rather than a queue), a higher priority than work items (if you have thousands of work items in the queue, a thread is likely to get a time slice earlier) or conversely if you want to leave the queue free for something else, etc.

• Enqueuing a work item on the thread pool is still a moderately expensive operation (above hundreds of nanoseconds). Besides the algorithm used to dynamically adjust the number of threads in the pool works best when each work item is not too long (let's say less than one millisecond). If your work items are longer than this or if they suffer a poor scalability (database or IO contention), you may want to limit the max number of threads in the pool.

• Some timers will enqueue their callbacks over the ThreadPool and some others on the UI. You need to check whether your timer will conflate with any other work you could have enqueued, especially long-running work items on the ThreadPool, and if it is important that your callbacks run before pending work items.

• Async/await are mostly necessary when the identity of the thread matters, typically for agents. For example the UI typically can only be manipulated from the UI thread (this is a thread affinity mechanism that serves as a synchronization pattern) and you often need to dispatch a task to a background thread from the UI thread then continue it later over the UI again. While on one hand your services look like agents, on the other hand it may be a design mistake as I just explained and anyway it does not seem like you need to dispatch continuations back to the service.

  However tasks are a nice abstraction to use and Parallel.Foreach, Task.Run and others will use the thread pool by default.

Edited the async/await section to take into account the comments below from Robert Harvey Edited to mention timers's callbacks.

Answer 2

Your design looks solid, although, the slowest point of the entire operation is going to be the reads from the database. Consider using an Async call, as this will free up the processor to perform other actions as it awaits the return call from the database.

Your Parallel.ForEach is definitely the best option, while it does use the thread pool (as noted), it also separates the queue from the thread pool, and has essentially one queue per processor thus there is no waiting in a lock block as they ask for the next task. Then when they finish their queue, they go to the next one and ask it for some (incurring a small hit to performance, but much less than if they all used the same queue.)

The only question you may want to think about is: what does the main thread do as the supporting threads are running? If it is doing nothing, then it is just eating up processor time that could be spent elsewhere. Even if you have it (the main thread) sleep until the other threads are done, it comes alive every so often only to sleep again, and if you use something like a ManualResetEvent, which waits until the other thread has completed to continue, then it is needlessly taking up memory. Have it do something, even if it turns into a servicer thread.

In short, Parallel.ForEach = Good!

Use Async and Await for database calls

Don't create unneeded threads.