Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :)"No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization. Or at least follow Eric's advice in above link and use Lazy<T>.

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization. Or at least follow Eric's advice in above link and use Lazy<T>.

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization. Or at least follow Eric's advice in above link and use Lazy<T>.

added 65 characters in body
Source Link
Nikita B
  • 13k
  • 1
  • 25
  • 57

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization. Or at least follow Eric's advice in above link and use Lazy<T>.

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization.

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization. Or at least follow Eric's advice in above link and use Lazy<T>.

added 602 characters in body
Source Link
Nikita B
  • 13k
  • 1
  • 25
  • 57

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPLTPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization.

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

RubberDuck is right. It looks like you are reinventing the wheel. Unless you are doing this whole thing for educational purposes, you should rather use TPL. ForEach method does exactly what you are trying to do and most likely does a better job at splitting processing power between the tasks:

Parallel.ForEach(source, action);

Regarding your implementation: CountdownEvent implements IDisposable, so you should dispose it after you are done using it. Or wrap it into a using block.

Also this code is the kind of micro-optimization you should avoid:

if (exceptions == null)
    lock (syncRoot)
        if (exceptions == null)
            exceptions = new ConcurrentBag<Exception>();

Is new ConcurrentBag call a bottleneack of this method? Does it take anywhere near as much resources as calling an Action method? "No? Then don't introduce additional concurrency!" :) Just call ConcurrentBag<Exception> exceptionsBag = new ConcurrentBag<Exception>(); and forget about synchronization.

Source Link
Nikita B
  • 13k
  • 1
  • 25
  • 57
Loading