Foreach loop and variables with locking

Question

Basically what I want to do is this psuedo code

List<DatabaseRecord> records;

List<ChangedItem> changedItems;

Parallel.ForEach<DatabaseRecord>(records, (item, loopState) =>
{
    if (item.HasChanged)
    {
        lock (changedItems)
        {
            changedItems.Add(new ChangedItem(item));
        }
    }
});

But what I'm worried about is locking on the changedItems. While it works, I have heard that it has to serialize the locked object over and over. Is there a better way of doing this?

Answer 1

I don't think that locking on the list will cause the list to serialize/deserialize as locking takes place on a private field available on all objects (syncBlockIndex). However, the recommended way to go about locking is to create a private field that you will specifically use for locking:

object _lock = new object();

This is because you have control over what you lock on. If you publish access to your list via a property, then code outside of your control might take a lock on that object and thus introduce a deadlock situation.

With regards to PLinq, I think deciding to use it depends on what your host and load is like. For example, in ASP.NET, PLINQ is more processor hungry which will get it done more quickly but at the expense of taking processing away from serving other web requests. The syntax is admittedly a lot cleaner.

Answer 2

Why don't you use PLinq instead? No locking needed:

changedItems = records.AsParallel()
                      .Where(x => x.HasChanged)
                      .Select(x => new ChangedItem(x))
                      .ToList();

Since you are projecting into a new list of ChangedItem and do not have any side effects this would be the way to go in my opinion.

Answer 3

It seems that you using this code in single thread ? if it's single thread , No lock needed . Does it works alright when you remove the line " lock (changedItems) " The code that @BrokenGlass have posted is more clear and easy to understand.

Answer 4

Could you use a ConcurrentCollection for your changedItems. Something like ConcurrentQueue? Then you wouldn't need to lock at all.

Update:

With regard to the ConcurrentQueue, the Enqueue won't block the thread in keeping the operation thread safe. It stays in user mode with a SpinWait...

public void Enqueue(T item)
{
    SpinWait wait = new SpinWait();
    while (!this.m_tail.TryAppend(item, ref this.m_tail))
    {
        wait.SpinOnce();
    }
}