question

njsokalski avatar image
0 Votes"
njsokalski asked njsokalski commented

Parallel.ForEach not always working

I have the following do loop:

 do
 {
 changed = false;
    
 //foreach (SquareSet ss in this.HorizontalSets) { changed = changed | ss.Solve(); }
 //foreach (SquareSet ss in this.VerticalSets) { changed = changed | ss.Solve(); }
    
 //Task.Run(() => { Parallel.ForEach(this.HorizontalSets, (ss) => { changed = changed | ss.Solve(); }); }).Wait();
 //Task.Run(() => { Parallel.ForEach(this.VerticalSets, (ss) => { changed = changed | ss.Solve(); }); }).Wait();
    
 //changed = changed | this.HorizontalSets.Select((ss) => { return ss.Solve(); }).Any((setchanged) => { return setchanged; });
 //changed = changed | this.VerticalSets.Select((ss) => { return ss.Solve(); }).Any((setchanged) => { return setchanged; });
    
 //changed = changed | this.HorizontalSets.AsParallel().Select((ss) => { return ss.Solve(); }).Any((setchanged) => { return setchanged; });
 //changed = changed | this.VerticalSets.AsParallel().Select((ss) => { return ss.Solve(); }).Any((setchanged) => { return setchanged; });
 } while (changed);

Each pair of statements executes the Solve() method on all the items in a List, and keeps a cumulative Boolean of the results. I want to do this using all CPUs, so I tried the 2nd & 4th techniques (using Parallel.ForEach & PLinq). The 1st, 3rd, & 4th techniques seem to always work, but the 2nd (the one using Parallel.ForEach) only seems to work sometimes. The first set of values (the values in this.HorizontalSets) must be finished before doing the second set of values (the values in this.VerticalSets). I think the code is somehow starting the second set before finishing all items in the first, but I am not exactly sure how to make sure it does not do this. Is there something I am doing wrong?

dotnet-csharp
5 |1600 characters needed characters left characters exceeded

Up to 10 attachments (including images) can be used with a maximum of 3.0 MiB each and 30.0 MiB total.

1 Answer

Viorel-1 avatar image
0 Votes"
Viorel-1 answered njsokalski commented

It seems that you did not try this yet:

 int c = 0;
 Parallel.ForEach( this.HorizontalSets, ss => { if( ss.Solve( ) ) Interlocked.Increment( ref c ); } );
 Parallel.ForEach( this.VerticalSets, ss => { if( ss.Solve( ) ) Interlocked.Increment( ref c ); } );
 changed = c != 0;

(In new .NET you can use Interlocked.Or).

It is not clear if you can also use a single ForEach:

 Parallel.ForEach( this.HorizontalSets.Concat( this.VerticalSets ), ss => { if( ss.Solve( ) ) Interlocked.Increment( ref c ); } );


· 5
5 |1600 characters needed characters left characters exceeded

Up to 10 attachments (including images) can be used with a maximum of 3.0 MiB each and 30.0 MiB total.

It is not clear if you can also use a single ForEach

No, I cannot concat this.HorizontalSets & this.VerticalSets, because I need to let the Solve() method FINISH on everything in this.HorizontalSets BEFORE I start working on the items in this.VerticalSets (I cannot work on the two sets at the same time).

But your suggestion of using Interlocked seems to work, so Thanks! One more question: Should I still use the Task.Run that is in the code I posted in my original post, or does that hurt or is unnecessary? Either way, thanks!

0 Votes 0 ·

I think that Parallel.ForEach creates the threads (tasks) for you, therefore Task.Run is not needed here.

By the way, consider a simplification:

 changed = false;
 Parallel.ForEach( this.HorizontalSets, ss => { if( ss.Solve( ) ) changed = true; } );
 Parallel.ForEach( this.VerticalSets, ss => { if( ss.Solve( ) ) changed = true; } );

Also make sure that Solve can be executed by parallel tasks (is "thread-safe").

0 Votes 0 ·

Thanks! So does that also mean that I don't actually need the Interlocked part (since your simplification does not have it)?

0 Votes 0 ·

Writing to bool does not need special measures according to documentation.

0 Votes 0 ·

Great! Thanks!

0 Votes 0 ·