Parallel.ForEach loop is performing like a serial loop

Question

I've spent about 8+ hours searching online for help and I couldn't find anything so, here goes.

I'm working with Team Foundation Server and C# and I'm trying to acquire a list of work items and convert them into a generic object we made to be bound to a special UI. To work items are Tasks for a specific day and the list is about 30ish items in size, so not that big of a deal.

The loop looks like this:

List<IWorkItemData> workitems = new List<IWorkItemData>();
var queryForData = Store.Query(query).Cast<WorkItem>();

if (queryForData.Count() == 0)
    return workitems;

Parallel.ForEach(queryForData, (wi) =>
{
    var temp = wi;
    lock (workitems)
    {
        TFSWorkItemData tfsWorkItem = new TFSWorkItemData(temp);
        workitems.Add(tfsWorkItem);
    }
});

The inside of TFSWorkItemData's constuctor looks like this:

public TFSWorkItemData(WorkItem workItem)
{
    this.workItem = workItem;

    this.Fields = new Dictionary<string, IFieldData>();

    // Add Fields
    foreach (Field field in workItem.Fields)
    {
        TFSFieldData fieldData = new TFSFieldData
        {
            Value = field.Value,
            OldValue = field.OriginalValue,
            ReferenceName = field.ReferenceName,
            FriendlyName = field.Name,
            ValueType = field.FieldDefinition.SystemType
        };
        this.Fields.Add(field.ReferenceName, fieldData);
    }
}

So it takes about 90 seconds to perform this operation. I know it doesn't take that long to grab 30 work items so it must be something I'm doing to cause this to take so long. I know that the lock is a performance hit, but when I remove it I get a InvalidOperationException saying that the collection has been modified. When I look in the details of this exception, the only useful info I can find is that the collection is a dictionary. What's weird is that it doesn't look to me like the field dictionary in the work item is being modified at all. And the dictionary in my class is only being added to and that shouldn't be the culprit unless I'm missing something.

Please help me figure out what I'm doing wrong regarding the dictionaries. I did try moving the parallel foreach loop to the workitem.Fields collection but I can't seem to get that to work.

Edit: Read comments of Answer for answer to this question. Thank you.

You're locking the list. Adding items to the list is the only thing that the parallel tasks do. But you're locking the list. Of _course_ it will be act like serial. — John Saunders, Jun 18 '15 at 22:18
Locking the list is the only thing that prevents the InvalidOperationException I'm getting. Thanks for the quick response and pointing out the error. — L_Laxton, Jun 18 '15 at 22:31
Keep in mind that Parallel.ForEach doesn't provide any guarantees as to how parallelized it'll be. — EJoshuaS - Stand with Ukraine, Apr 05 '17 at 15:16

Yuval Itzchakov · Answer 1 · 2015-06-18T22:37:15.123

1

Please help me figure out what I'm doing wrong regarding the dictionaries.

The exception is thrown because List<T> is not thread-safe.

You have a shared resource which needs to be modified, using Parallel.ForEach won't really help, as you're moving the bottleneck to the lock, causing the contention there, which is likely as to why you're seeing performance actually degrade. Threads aren't a magical solution. You need to aspire to have as many independent workers which can each do their own work.

Instead, you can try using PLINQ which will partition your enumerable internally. Since you actually want to project each element in the collection, you can use Enumerable.Select:

 var workItems = queryForData.AsParallel().Select(workItem => new TFSWorkItemData(workItem)).ToArray();

In order to find out if this solution is actually better than sequential iteration, benchmark your code. Never assume more threads are faster.

edited Jun 18 '15 at 22:37

answered Jun 18 '15 at 22:29

Yuval Itzchakov

146,575
32
257
321

This is much better, but still may not improve performance much. I doubt there's really that much going on in the `TFSWorkItemData` constructor, unless accessing the fields is reading from TFS. – John Saunders Jun 18 '15 at 22:34
@JohnSaunders I agree, that's why I said benchmarking will be the only truth. If this is a simple, small iteration, that won't help either. – Yuval Itzchakov Jun 18 '15 at 22:35
Thanks a lot I'll try that. – L_Laxton Jun 18 '15 at 22:36
1

Looks like I'm getting the same InvalidOperationException so maybe John is right and accessing the fields in the work item is reading from TFS. I appreciate the better solution though. – L_Laxton Jun 18 '15 at 22:46
Okay I think John was right. According to [this site](https://msdn.microsoft.com/en-us/library/bb130306.aspx), in the Select section of the table it says if the field being accessed is not in the query (and many of them aren't) it will require another round trip to the database. So that is what I think is modifying the collection. Thanks for your help guys. – L_Laxton Jun 18 '15 at 23:09
@L_Laxton: please show us exactly which line threw the `InvalidOperationException` – John Saunders Jun 18 '15 at 23:55

L_Laxton · Accepted Answer · 2015-10-29T22:42:31.857

0

I found a another way that might work for anyone trying to do something similar.

// collect all of the IDs
var itemIDs = Store.Query(query).Cast<WorkItem>().Select(wi = wi.Id).ToArray();

IWorkItemData[] workitems = new IWorkItemData[itemIDs.Length];

// then go through the list, get the complete workitem, create the wrapper,
// and finally add it to the collection
System.Threading.Tasks.Parallel.For(0, itemIDs.Length, i =>
{
    var workItem = Store.GetWorkItem(itemIDs[i]);
    var item = new TFSWorkItemData(workItem);
    workitems[i] = item;
});

Edit: changed list to array

edited Oct 29 '15 at 22:42

answered Jul 07 '15 at 23:41

L_Laxton

67
1
7

Your "provided solution" isn't thread safe. You're appending to a `List` in parallel. – Yuval Itzchakov Jul 08 '15 at 05:44
You're right it wasn't. Hope my recent edit fixes things. This is pretty much how I'm doing it in my code now. – L_Laxton Oct 29 '15 at 22:43

Parallel.ForEach loop is performing like a serial loop

2 Answers2