How to execute Parallel.ForEach correctly, block and track progress

Question

I am trying to implement a Parallel.ForEach pattern and track the progress, but I am missing something regarding blocking. The following example counts to 1000 when threadCount = 1 , but not when threadCount > 1. What is the correct way to do this?

 class Program { static void Main() { var progress = new Progress(); var ids = Enumerable.Range(1, 10000); var threadCount = 2; Parallel.ForEach(ids, new ParallelOptions { MaxDegreeOfParallelism = threadCount }, id => { progress.CurrentCount++; }); Console.WriteLine("Threads: {0}, Count: {1}", threadCount, progress.CurrentCount); Console.ReadKey(); } } internal class Progress { private Object _lock = new Object(); private int _currentCount; public int CurrentCount { get { lock (_lock) { return _currentCount; } } set { lock (_lock) { _currentCount = value; } } } } 

Answer 1

A common problem with calling something like count++ from multiple threads (which share the count variable) is that this sequence of events can happen:

• Thread A reads the value of count .
• Thread B reads the value of count .
• Thread A increases its local copy.
• Thread B increases its local copy.
• Thread A writes the value with an extra value to count .
• Thread B writes the incremental value back to count .

Thus, the value recorded by stream A is overwritten by stream B, so the value actually increases only once.

Your code adds locks around operations 1, 2 ( get ) and 5, 6 ( set ), but it does nothing to prevent a problematic sequence of events.

What you need to do is block the entire operation, so that although thread A increases the value, thread B cannot access it at all:

 lock (progressLock) { progress.CurrentCount++; } 

If you know that you only need to increase, you can create a method on Progress that encapsulates this.

Answer 2

Old question, but I think there is a better answer.

You can report progress using Interlocked.Increment(ref progress) so that you don’t have to worry about performing a write operation.

Answer 3

The simplest solution is to replace the property with a field, and

 lock { ++progress.CurrentCount; } 

(I personally prefer the appearance of preincrement over postincrease, since the “++.” Thing collides in my mind! But postincrementation, of course, will work the same way.)

This will have the added benefit of reducing overhead and competition, since updating a field is faster than calling a method that updates the field.

Of course, encapsulation as a property can also have advantages. IMO, since the syntax of the field and the property is identical, ONLY the advantage of using the property over the field when the property is auto-updated or equivalent is when you have a script in which you might want to deploy one assembly without having to create and deploy dependent assemblies anew. Otherwise, you can use faster fields! If you need to check the value or add a side effect, you simply convert this field to a property and create it again. Therefore, in many practical cases there is no penalty for using the field.

However, we live in a time when many development teams work dogmatically and use tools like StyleCop to ensure their dogmatism. Such tools, unlike coders, are not smart enough to judge when using a field is acceptable, therefore invariably “the rule is simple enough to check even StyleCop”, it becomes “encapsulate fields as properties”, “do not use public fields”, et cetera. ..

Answer 4

Removing lock statements from properties and changing the main body:

  object sync = new object(); Parallel.ForEach(ids, new ParallelOptions {MaxDegreeOfParallelism = threadCount}, id => { lock(sync) progress.CurrentCount++; }); 

Answer 5

The problem here is that ++ not atomic - one thread can read and increment a value between another thread that reads a value, and it saves the (now incorrect) increment of the value. This is probably compounded by the fact that the property wraps your int .

eg.

 Thread 1 Thread 2 reads 5 . . reads 5 . writes 6 writes 6! . 

Locks around the setter and getter do not help this, since there is nothing to stop; lock blocks the shutdown of these lines.

I usually suggest using Interlocked.Increment , but you cannot use this with a property.

Instead, you can open _lock and have a lock block around the call to progress.CurrentCount++; .

Answer 6

It is better to store any database or file system operation in a local buffer variable instead of locking it. blocking reduces performance.