What is the correct usage of ConcurrentBag?

Question

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?

Solution 1

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.

Solution 2

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.

Comments

[Ilya Ivanov]

I think overhead and overall performance will be greater in this scenario, than using synchronous approach. Have you measured it?

[Dustin Kingen]

ConcurrentBag has a constructor that takes an IEnumerable<T> var myBag = new ConcurrentBag<Product>(products);

[hisoka21]

Hi llya, what I mean is that I'll still almost the same code above except that I will be replacing the ConcurrentBag<Product> with a List<Product> and add a lock when adding a product in the List inside the Parallel.ForEach

[Scott Chamberlain]

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.

[Voo]

"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]

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