3
\$\begingroup\$

I feel like there is a better way of writing this code which waits for multiple threads to finish their work (i.e. without using two loops), but my colleagues and I are drawing a blank.

private static void ProcessRecords(long[] recordIds)
{
    var threads = new List<Thread>();
    var recordsPerThread = recordIds.Length / Settings.Default.WorkerThreads + 1;

    for (var i = 0; i < Settings.Default.WorkerThreads; i++)
    {
        var t = new Thread(new ParameterizedThreadStart(Program.SetOwner));
        threads.Add(t);
        t.Start(recordIds.Skip(i * recordsPerThread).Take(recordsPerThread));
    }

    // Wait until all threads have completed
    foreach (var t in threads)
    {
        t.Join();
    }
}

private static void SetOwner(object o)
{
    var recordIds = o as IEnumerable<long>;

    using (Database d = Program.GetDatabase())
    {
        Account owner = Program.GetOwner(d);

        foreach (long id in recordIds)
        {
            try
            {
                var record = d.GetRecordById(id);
                record.Owner = owner;
                record.Save();
            }
            catch (Exception ex)
            {
                // code to log error
            }
        }
    }
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Do you have a particular reason for using threads directly, rather than the Task Parallel Library? \$\endgroup\$ Sep 7 '17 at 9:59
  • \$\begingroup\$ Yes, I tried using PLINQ at first, but the Program.SetOwner method opens a third-party "database" connection (which cannot be shared across threads) and so each thread needed it's own connection, our tests showed that that was slower than using a single thread to process the records. By using the code above each thread has it's own connection and deals with a "fair share" of the records. I am open to other suggestions though. \$\endgroup\$
    – Vitani
    Sep 7 '17 at 10:25
  • \$\begingroup\$ I'm pretty sure you could use PLINQ but it would require adjusting the SetOwner method and the database connection. Could you post this one too? \$\endgroup\$
    – t3chb0t
    Sep 7 '17 at 11:21
  • \$\begingroup\$ So I assume you had something like recordIds.AsParallel().ForAll(Program.SetOwner) before? \$\endgroup\$ Sep 7 '17 at 11:40
  • \$\begingroup\$ @t3chb0t I've added the Program.SetOwner code to the question. @PieterWitvoet That's correct, along a dash of WithDegreeOfParallelism() \$\endgroup\$
    – Vitani
    Sep 7 '17 at 14:30
4
\$\begingroup\$

So your original approach looked something like this:

recordIds.AsParallel()
    .WithDegreeOfParallelism(...)
    .ForAll(id => SetOwner(id));

which involved a separate call to SetOwner for each record. Because SetOwner has a lot of overhead per call, it's indeed much better to let it operate on batches rather than individual items. But you don't need to use threads for that directly:

recordIds
    .SplitIntoBatches(Settings.Default.WorkerThreads)
    .AsParallel()
    .WithDegreeOfParallelism(Settings.Default.WorkerThreads)
    .ForAll(batch => SetOwner(batch));


public static IEnumerable<IEnumerable<T>> SplitIntoBatches<T>(this IEnumerable<T> items, int numberOfBatches)
{
    var batchSize = (items.Count() + numberOfBatches - 1) / numberOfBatches;
    for (int i = 0; i < numberOfBatches; i++)
        yield return items.Skip(i * batchSize).Take(batchSize);
}
\$\endgroup\$
4
  • \$\begingroup\$ A 2D array, I like your thinking! I will give that a go and report back, many thanks. \$\endgroup\$
    – Vitani
    Sep 7 '17 at 15:36
  • \$\begingroup\$ Seems to work well, and looks nicer, thanks! \$\endgroup\$
    – Vitani
    Sep 8 '17 at 8:42
  • \$\begingroup\$ Nice idea to make it generic & yield (I've never actually written function which yields before), but can you explain batchSize = (items.Count() + numberOfBatches - 1) / numberOfBatches please? It doesn't seem to make sense to me. \$\endgroup\$
    – Vitani
    Sep 8 '17 at 9:29
  • 1
    \$\begingroup\$ It does the same as your +1, but only if the item count is not exactly divisible by the number of batches. \$\endgroup\$ Sep 8 '17 at 9:36
3
\$\begingroup\$

It's so hard to parallelize this becasue SeOwner has a hardcoded database dependency that it shouldn't. In fact it should get it via a parameter like GetOwner does. But not only this, it should also recieve the owner via a parameter.

The very simple version of it should just do the job of setting the owner, nothing else, no database creations, not looping etc:

private static void SetOwner(object database, Account owner, long recordId)
{
    try
    {
        var record = database.GetRecordById(recordId);
        record.Owner = owner;
        record.Save();
    }
    catch (Exception ex)
    {
        // code to log error
    }
}

Having this you can now update your code processing the records and use the Parallel.ForEach. With localInit your first create a database instance per thread and with localFinally you dispose it. Then on each loop within the same thread body gets called and the local is passed to it along with the recordId and a loopState (that allows you to stop/break the loop if necessary). For easier debugging you can set MaxDegreeOfParallelism to 1.

Parallel.ForEach
(
    source: recordIds,
    parallelOptions: new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount },
    localInit: () => new { Database = Program.GetDatabase() },
    body: (recordId, loopState, local) =>
    {
        SetOwner(local.Database, GetOwner(local.Database), recordId);
        return local;
    },
    localFinally: local => local.Database.Dispose()
);
\$\endgroup\$
8
  • \$\begingroup\$ If I understand your code correctly, then this shares a database connection across all the threads, which the third-party code does not support (it throws an Exception). \$\endgroup\$
    – Vitani
    Sep 8 '17 at 8:15
  • \$\begingroup\$ @Jocie nope, it uses one connection per thread. \$\endgroup\$
    – t3chb0t
    Sep 8 '17 at 8:31
  • \$\begingroup\$ Ah, we tried that and it was slower than using a single thread. Thanks for your input though! \$\endgroup\$
    – Vitani
    Sep 8 '17 at 8:49
  • \$\begingroup\$ @Jocie this is of course always possible. Most of the time it's hard to predict whether a parallel implementation will really be faster so your approach by trying different techniques was correct ;-) \$\endgroup\$
    – t3chb0t
    Sep 8 '17 at 8:51
  • \$\begingroup\$ It might be slower because the original code only calls GetOwner once per batch, while this calls it for every record. Moving that call into localInit should help. \$\endgroup\$ Sep 8 '17 at 8:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.