# Critique my JobQueue class

This topic is 2950 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

The purpose: Execute jobs on one or more threads.

What I'm looking for: Critique, comments, improvements, things to look at.

The implementation:
 /// <summary> /// Runs queued jobs in a threaded environment. Thread-safe. /// </summary> public class JobQueue { /// <summary> /// A queue of jobs. /// </summary> System.Collections.Concurrent.ConcurrentQueue<Action> queue; /// <summary> /// An action to call when there are no more jobs left. /// </summary> Action whenAllJobsDone; /// <summary> /// Creates a new job queue. /// </summary> /// <param name="whenAllJobsDone">If queue.Count is noticed to be at 0, this action will be carried out and the jobqueue will terminate.</param> public JobQueue(Action whenAllJobsDone) { queue = new System.Collections.Concurrent.ConcurrentQueue<Action>(); this.whenAllJobsDone = whenAllJobsDone; } /// <summary> /// Creates a new job queue. /// </summary> /// <param name="whenAllJobsDone">If queue.Count is noticed to be at 0, this action will be carried out and the jobqueue will terminate.</param> /// <param name="queue">A user-provided queue of jobs; this allows quick and easy sharing of jobs among more than one JobQueue.</param> public JobQueue(Action whenAllJobsDone, System.Collections.Concurrent.ConcurrentQueue<Action> queue) { this.queue = queue; this.whenAllJobsDone = whenAllJobsDone; } /// <summary> /// Enqueues a new job. /// </summary> /// <param name="value">The job action.</param> public void Enqueue(Action value) { queue.Enqueue(value); } /// <summary> /// Enumerates and enqueues a range of new jobs.. /// </summary> /// <param name="values">The range of new jobs.</param> public void EnqueueRange(IEnumerable<Action> values) { foreach (Action a in values) queue.Enqueue(a); } /// <summary> /// Tries to dequeue a job from the queue. This allows a monitoring thread to do some of the work. /// </summary> /// <param name="a">A job, if one is dequeued, will be placed here.</param> /// <returns>Returns true if a job was dequeued.</returns> public bool TryDequeue(out Action a) { return queue.TryDequeue(out a); } /// <summary> /// Starts a new thread and runs the job queue on it. /// </summary> public void Start() { Thread thread = new Thread(Run); thread.IsBackground = true; thread.Start(); } /// <summary> /// Runs the job queue on the calling thread. /// </summary> public void Run() { Action a; while (queue.Count > 0) { if (queue.TryDequeue(out a)) a(); } whenAllJobsDone(); } } 
Thanks.

##### Share on other sites
Are you supposed to be able to add jobs to a single JobQueue object from different threads? i.e. if ThreadA and ThreadB each have a reference to the same JobQueue object, is it supposed to be safe for ThreadA and ThreadB to add jobs to the queue? Because that's what I'm seeing when I see "Thread-safe" in JobQueue's summary, but that's not what I'm seeing from the code.

##### Share on other sites
My question would be what is its intended usage pattern?

A quick look at the code seems to be a case of 'add jobs to queue, then spin up thread to execute them one at a time, wait for thread to signal it is done at which point the thread dies'...

Why do this instead of using say a thread pool where you could get your tasks spread across multiple threads? or even just spin up a single thread and have it wait for work to be queued to do it if you really want a single thred to process a whole queue.

##### Share on other sites
In order:

@Cornstalks: .Net calls the queue used a "ConcurrentQueue" and I'm taking them at their word. If it's not actually concurrent, I'd like to know how.

@Phantom: For a thread pool - Isn't starting a thread slow? As for spinning up a single thread and having it wait for work, how would you code that, other than with a class that simply does jobs until told to exit, in which case, a different type of JobQueue class seems logical?

Thanks. It seems you might have better answers, but they need to be explained more.

##### Share on other sites
Yes, starting a thread is slow, however based on the code above that is what you do every time you want to process a group of work. You can't start the thread before you've got any work as it would just exit right away after all.

A thread pool starts up a bunch of threads and then has them take work as it appears in the work queue. The loop, per thread, wouldn't be that different from your current thread run, except that it would have to wait on one, maybe two, event(s) (a 'work ready' and an 'exit' event) instead of looking at the length of the queue. When work is pushed into the (shared) queue an event would be triggered which would wake up a thread and have it work on the work before going back to sleep again and waiting for another bit of work. (You can play around with the concept a bit, have threads which are already awake recheck for work before waiting on the event again for example).

This is why however I asked about the intended usage pattern; if the intent was to queue up work and then start a thread to perform it and have it terminated then your (over all) design is fine. If, however, you wanted to constantly add work to a queue to be done by one, or more threads, then the cost of starting the thread to process each time probably isn't what you want to pay.

##### Share on other sites

Yes, starting a thread is slow, however based on the code above that is what you do every time you want to process a group of work. You can't start the thread before you've got any work as it would just exit right away after all.

A thread pool starts up a bunch of threads and then has them take work as it appears in the work queue. The loop, per thread, wouldn't be that different from your current thread run, except that it would have to wait on one, maybe two, event(s) (a 'work ready' and an 'exit' event) instead of looking at the length of the queue. When work is pushed into the (shared) queue an event would be triggered which would wake up a thread and have it work on the work before going back to sleep again and waiting for another bit of work. (You can play around with the concept a bit, have threads which are already awake recheck for work before waiting on the event again for example).

This is why however I asked about the intended usage pattern; if the intent was to queue up work and then start a thread to perform it and have it terminated then your (over all) design is fine. If, however, you wanted to constantly add work to a queue to be done by one, or more threads, then the cost of starting the thread to process each time probably isn't what you want to pay.

Thanks; that's informative. The thread pool idea sounds more useful for the sorts of things I'm thinking on (AI tasks (pathfinding, graph search), mainly).

• ### Game Developer Survey

We are looking for qualified game developers to participate in a 10-minute online survey. Qualified participants will be offered a \$15 incentive for your time and insights. Click here to start!

• 9
• 11
• 15
• 21
• 26