Performance optimization of foreach loop in C#

Question

I've got a method:

IList<string> pages = new List<string>();
foreach (var node in nodes)
{
    try
    {
        string temp = DoSomeComplicatedModificationOnNode(node);
        if (temp.ToLower().Contains(path))
        {
            pages.Add(node.Title);
        }
    }
    catch (Exception)
    {
        continue;
    }
}

DoSomeComplicatedModificationOnNode() gives exception in some cases, that's why the try{} catch block is used - I can skip the items which gives exception. The number of nodes contains several thousands of items, an item has several properties. How can I optimize this loop? I was thinking about Parallel.Foreach, but the following code gives me an error "Missing current principal":

IList<string> pages = new List<string>();
Parallel.ForEach(pageNodes, node =>
{
    try
    {
        string temp = DoSomeComplicatedModificationOnNode(node);
        if (temp.ToLower().Contains(path))
        {
            pages.Add(node.Title);
        }
    }
    catch (Exception)
    {
    }
});

Answer 1

In C#, generic list are not thread-safe, so you can not add a items in a parallel loop.

I recommend using another class like ConcurrentBag, ConcurrentStack or ConcurrentQueue.

var pages = new ConcurrentBag<string>();
Parallel.ForEach(pageNodes, node =>
{
    try
    {
        string temp = DoSomeComplicatedModificationOnNode(node);
        if (temp.ToLower().Contains(path))
            pages.Add(node.Title);
    }
    catch (Exception)
    {
        throw;
    }
});

Remember that parallel tasks are disordered, if you want an order you will have to use an index in Parallel. List are only thead-save for reading.

System.Threading.Tasks.Parallel.For(0, pageNodes.Count, index =>
{
    string node = pageNodes[index];

    try
    {
        string temp = DoSomeComplicatedModificationOnNode(node);
        if (temp.ToLower().Contains(path))
            pages.Add(MyPage(index, node.Title));
    }
    catch (Exception)
    {
        throw;
    }
});

Answer 2

I would recommend to use PLINQ for such purposes. Parallel LINQ is a parallel implementation of LINQ and has the same set of operations. Code written using PLINQ follows functional style rules - there is no any updates, just mapping of current list in parallel mode. It can increase performance for your case by running a mappers in different threads and then gather result in one single "dataset". Of course it can boost performance only in the case you have CPU with few core (but as usual nowadays we all have few cores).

Here is an example

    private static void Main(string[] args)
    {
        var result =
            GenerateList()
                .AsParallel()
                .Select(MapToString)
                .Where(x => !String.IsNullOrWhiteSpace(x))
                .ToList();

        Console.ReadKey();
    }

    private const string Path = "1";
    private static string MapToString( int node)
    {
        //Console.WriteLine("Thread id: {0}", Thread.CurrentThread.ManagedThreadId);
        try
        {
            string temp = DoSomeComplicatedModificationOnNode(node);
            if (temp.ToLower().Contains(Path))
            {
                return temp;
            }
        }
        catch (Exception)
        {
            return null;
        }

        return null;
    }
    private static IEnumerable<int> GenerateList()
    {
        for (var i=0; i <= 10000; i++)
            yield return i;
    }

    private static string DoSomeComplicatedModificationOnNode(int node)
    {
        return node.ToString(CultureInfo.InvariantCulture);
    }

Answer 3

List<T> is in most cases not thread-safe. Have a look at the thread-safe collections like e.g. ConcurrentBag<T>.

Answer 4

The real performance problem is that you are catching the exceptions, just try to notify the result using variables.

For long running methods you should use asyncronous (asyc/await) methods.

Beware with parallel operations (in LINQ user AsParallel()) because your resources are limited and you could be without memory when you least expect it. And your code must be thread-safe (List is not thread-safe).

I would bet that you will not have a better performance than the following code:

var pages = nodes.Select(x => new { Status = DoSomeComplicatedModificationOnNode(x), Node = x })
    .Select(x => x?.Result)
    .Where(x => x.Status.IsCorrect && x.Status.ToLowerInvariant().Contains(path))
    .Select(x => x.Node.Title)
    .ToList();

Or using asyncronous + LINQ:

var pages = nodes.Select(async x =>
{
    return new { Status = await DoSomeComplicatedModificationOnNode(x), Node = x };
})
.Select(x => x?.Result)
.Where(x => x.Status.IsCorrect && x.Status.ToLowerInvariant().Contains(path))
.Select(x => x.Node.Title)
.ToList();

Until the call to ToList the entire query will not be executed.

Answer 5

No, using multiple threads will likely not make your loop any faster. There are three obvious mistakes in your code, that make it run slower.

1. The pages list will be re-allocated multiple times.
2. The call to ToLower() creates a temporary string on each run.
3. The DoSomeComplicatedModificationOnNode might throw.

See fixed version below.

// Give the list an initial capacity (best guess), avoiding re-allocations.
var pages = new List<string>(nodes.Length);
foreach (var node in nodes)
{
    string temp = DoSomeComplicatedModificationOnNode(node, out var error);
    if (error != null)
    {
        continue;
    }

    // IndexOf() allows for allocation-free case insensitive string search.
    if (temp.IndexOf(path, StringComparison.CurrentCultureIgnoreCase) >= 0)
    {
        pages.Add(node.Title);
    }
}