66

I've already read previous questions here about ConcurrentBag but did not find an actual sample of implementation in multi-threading.

ConcurrentBag is a thread-safe bag implementation, optimized for scenarios where the same thread will be both producing and consuming data stored in the bag."

Currently this is the current usage in my code (this is simplified not actual codes):

private void MyMethod()
{
    List<Product> products = GetAllProducts(); // Get list of products
    ConcurrentBag<Product> myBag = new ConcurrentBag<Product>();

    //products were simply added here in the ConcurrentBag to simplify the code
    //actual code process each product before adding in the bag
    Parallel.ForEach(
                products,
                new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount },
                product => myBag.Add(product));

    ProcessBag(myBag); // method to process each items in the concurrentbag
}

My questions:
Is this the right usage of ConcurrentBag? Is it ok to use ConcurrentBag in this kind of scenario?

For me I think a simple List<Product> and a manual lock will do better. The reason for this is that the scenario above already breaks the "same thread will be both producing and consuming data stored in the bag" rule.
Also I also found out that the ThreadLocal storage created in each thread in the parallel will still exist after the operation (even if the thread is reused is this right?) which may cause an undesired memory leak.
Am I right in this one guys? Or a simple clear or empty method to remove the items in the ConcurrentBag is enough?

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
hisoka21
  • 855
  • 1
  • 12
  • 21
  • I think overhead and overall performance will be greater in this scenario, than using synchronous approach. Have you measured it? – Ilya Ivanov Mar 20 '13 at 10:58
  • 1
    ConcurrentBag has a constructor that takes an `IEnumerable` `var myBag = new ConcurrentBag(products);` – Dustin Kingen Mar 20 '13 at 11:55
  • Hi llya, what I mean is that I'll still almost the same code above except that I will be replacing the ConcurrentBag with a List and add a lock when adding a product in the List inside the Parallel.ForEach – hisoka21 Mar 20 '13 at 13:28
  • 1
    Because you are not using the same threads to add to the bag as will be removing the bag you may have better performance from a ConcurrentQueue instead of a ConcurrentBag. – Scott Chamberlain Oct 27 '17 at 19:21

4 Answers4

27

This looks like an ok use of ConcurrentBag. The thread local variables are members of the bag, and will become eligible for garbage collection at the same time the bag is (clearing the contents won't release them). You are right that a simple List with a lock would suffice for your case. If the work you are doing in the loop is at all significant, the type of thread synchronization won't matter much to the overall performance. In that case, you might be more comfortable using what you are familiar with.

Another option would be to use ParallelEnumerable.Select, which matches what you are trying to do more closely. Again, any performance difference you are going to see is likely going to be negligible and there's nothing wrong with sticking with what you know.

As always, if the performance of this is critical there's no substitute for trying it and measuring.

bmm6o
  • 6,187
  • 3
  • 28
  • 55
9

It seems to me that bmm6o's is not correct. The ConcurrentBag instance internally contains mini-bags for each thread that adds items to it, so item insertion does not involve any thread locks, and thus all Environment.ProcessorCount threads may get into full swing without being stuck waiting and without any thread context switches. A thread sinchronization may require when iterating over the collected items, but again in the original example the iteration is done by a single thread after all insertions are done. Moreover, if the ConcurrentBag uses Interlocked techniques as the first layer of the thread synchronization, then it is possible not to involve Monitor operations at all.

On the other hand, using a usual List<T> instance and wrapping each its Add() method call with a lock keyword will hurt the performance a lot. First, due to the constant Monitor.Enter() and Monitor.Exit() calls that each require to step deep into the kernel mode and to work with Windows synchronization primitives. Secondly, sometimes occasionally one thread may be blocked by the second thread because the second thread has not finished its addition yet.

As for me, the code above is a really good example of the right usage of ConcurrentBag class.

Sergey Volodko
  • 483
  • 5
  • 5
  • 4
    "that each require to step deep into the kernel mode" yeah no. This is another nice example of why it's important to measure performance and not just theorise about it. The chances of the being a single kernel call for a lock that does nothing but add an element to a list is minimal. – Voo Feb 22 '19 at 08:47
  • 3
    the only way a kernel call happens is if there's lock convention (unlikely already if non negligible work is done in each thread) *and* the lock is held longer than the spin count (for a single add this is again tremendously unlikely). But again don't trust me, *measure* – Voo Feb 22 '19 at 08:51
1

Is this the right usage of ConcurrentBag? Is it ok to use ConcurrentBag in this kind of scenario?

No, for multiple reasons:

  1. This is not the intended usage scenario for this collection. The ConcurrentBag<T> is intended for mixed producer-consumer scenarios, meaning that each thread is expected to add and take items from the bag. Your scenario is nothing like this. You have many threads that add items, and zero threads that take items. The main application for the ConcurrentBag<T> is for making object-pools (pools of reusable objects that are expensive to create or destroy). And given the availability of the ObjectPool<T> class in the Microsoft.Extensions.ObjectPool package, even this niche application for this collection is contested.
  2. It doesn't preserve the insertion order. Even if preserving the insertion order is not important, getting a shuffled output makes the debugging more difficult.
  3. It creates garbage that has to be collected by the GC. It creates one WorkStealingQueue (internal class) per thread, each containing an expandable array, so the more threads you have the more objects you allocate. Also each time it is enumerated it copies all the items in a new array, and exposes an IEnumerator<T> GetEnumerator() property that is boxed on each foreach.
  4. There are better options available, offering both better performance and better ordering behavior.

In your scenario you can store the results of the parallel execution in a simple array. Just create an array with length equal to the products.Count, switch from the Parallel.ForEach to the Parallel.For, and assign the result directly to the corresponding slot of the results array without doing any synchronization at all:

List<Product> products = GetAllProducts(); // Get list of products
Product[] results = Product[products.Count];

Parallel.For(0, products.Count,
    new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount },
    i => results[i] = products[i]);

ProcessResults(results);

This way you'll get the results with perfect ordering, stored in a container that has the most compact size and the fastest enumeration of all .NET collections, doing only a single object allocation.

In case you are concerned about the thread-safety of the above operation, there is nothing to worry about. Each thread writes on different slots in the results array. After the completion of the parallel execution the current thread has full visibility of all the values that are stored in the array, because the TPL includes the appropriate barriers when tasks are queued, and at the beginning/end of task execution (citation).

(I have posted more thoughts about the ConcurrentBag<T> in this answer.)

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
-1

If List<T> is used with a lock around Add() method it will make threads wait and will reduce the performance gain of using Parallel.ForEach()

Eric Aya
  • 69,473
  • 35
  • 181
  • 253