-3

Using the command pattern returning tasks that are just sleeps for 5 seconds the total for the 3 tasks to complete is ~15 seconds.

What am I doing that is keeping this code from executing in "parallel"?

The calling code

        var timer = new Stopwatch();

        timer.Start();

        var task = CommandExecutor.ExecuteCommand(new Fake());
        var task2 = CommandExecutor.ExecuteCommand(new Fake());
        var task3 = CommandExecutor.ExecuteCommand(new Fake());

        Task.WaitAll(new Task[]
        {
            task, task2, task3
        });

        timer.Stop();

        Debug.Print("{0}ms for {1},{2},{3} records", timer.ElapsedMilliseconds, task.Result.Count, task2.Result.Count(), task3.Result.Count());

The Commands being executed

public class Fake : Command<Task<Dictionary<string, string[]>>>
{
    public override string ToString()
    {
        return string.Format("Fake");
    }

    protected override void Execute()
    {
        new System.Threading.ManualResetEvent(false).WaitOne(5000);

        Result = Task.Factory.StartNew(() => new Dictionary<string, string[]>());
    }
}

The Command Abstraction

public abstract class Command
{
    public void Run()
    {
        try
        {
            var timer = new Stopwatch();
            timer.Start();
            //Debug.Print("{0}-{1}", ToString(), "Executing");
            Execute();
            timer.Stop();
            Debug.Print("{0}-{1} Duration: {2}ms", ToString(), "Done", timer.ElapsedMilliseconds.ToString(CultureInfo.InvariantCulture));             
        }
        catch (Exception ex)
        {
            Debug.Print("Error processing task:" + ToString(), ex);
        }
    }

    public abstract override string ToString();

    protected abstract void Execute();
}

/// <summary>
/// A command with a return value
/// </summary>
/// <typeparam name="T"></typeparam>
public abstract class Command<T> : Command
{
    public T Result { get; protected set; }

    public T GetResult()
    {
        Run();
        return Result;
    }
}

the Command Executor

public class CommandExecutor
{
    /// <summary>
    ///     Executes the command.
    /// </summary>
    /// <param name="cmd">The CMD.</param>
    public static void ExecuteCommand(Command cmd)
    {
        cmd.Run();
    }

    /// <summary>
    ///     Executes the command for commands with a result.
    /// </summary>
    /// <typeparam name="TResult">The type of the result.</typeparam>
    /// <param name="cmd">The CMD.</param>
    /// <returns></returns>
    public static TResult ExecuteCommand<TResult>(Command<TResult> cmd)
    {
        ExecuteCommand((Command) cmd);

        return cmd.Result;
    }
Steve
  • 25,806
  • 2
  • 33
  • 43
  • 1
    You should post the relevant code here. If it's too much for you to post here then you should be simplifying the example. – Servy Jan 16 '13 at 19:46
  • I believe there's a work item scheduler behind the scenes that has a heuristic for working out when to run tasks in parallel or not, create a new thread, end an existing thread etc. If you create 1,000,000 tasks it doesn't create that many threads straight away, it figures out how to best execute them because that many threads would kill the CPU. A task that takes 5 seconds to complete might not be the best for this heuristic. See http://stackoverflow.com/questions/3105988/taskcreationoptions-longrunning-option-and-threadpool for more info. – ta.speot.is Jan 16 '13 at 19:52
  • 1) You haven't even provided all of the code. You haven't provided the definitions of `CommandExecutor` or `MoreFakes`. We should be able to copy-paste an example such as this and run it. 2) You have *way* too much unneeded code here. The vast majority of this doesn't actually matter. The point point of using `Task` objects is that you don't need these types of abstractions. – Servy Jan 16 '13 at 19:53
  • @ta.speot.is That's not the issue here. He's coded it in such a way that they can't possibly be parallelized at all. – Servy Jan 16 '13 at 19:55
  • @Servy upvotes for you, then. – ta.speot.is Jan 16 '13 at 19:56
  • @Servy i updated the code so hopefully it should run. I was just cutting snippets from my actual tests. Sorry I forgot commandexecutor and morefakes was just more of the same of fakes so i changed the name to be the same. I think you are correct though about my placement of the sleep. Sorry for my mistakes in the question. Ya'll are quick on the down vote. – Steve Jan 16 '13 at 20:09

1 Answers1

3

The problem is that you aren't waiting inside the actual Task object, you're waiting in the method that creates the task before it actually provides that task:

protected override void Execute()
{
    new System.Threading.ManualResetEvent(false).WaitOne(5000);

    Result = Task.Factory.StartNew(() => new Dictionary<string, string[]>());
}

should instead be:

protected override void Execute()
{
    Result = Task.Factory.StartNew(() =>
    {
        new System.Threading.ManualResetEvent(false).WaitOne(5000);
        return new Dictionary<string, string[]>();
    });
}
Servy
  • 202,030
  • 26
  • 332
  • 449
  • 1
    @Steve Which is why I told you right off the bat that you were using *way* too much unneeded code. You should just have a single static method that returns a task (basically this method but returning the task) and then a `WaitAll` on three instances of that. That's all you need to demonstrate this issue. – Servy Jan 16 '13 at 20:15
  • yeah it was easy to blame the other code when the problem was so simple. – Steve Jan 16 '13 at 20:18
  • @Steve That's one of the reasons why you should simplify your code as much as you can before posting a question: you might find where the error actually is during that. – svick Jan 16 '13 at 20:32