[phobos] std.parallelism: Request for review/comment

[Sean Kelly]

A few general comments:

I'm wondering whether the use of a TaskPool should always be explicit.  Perhaps a default pool should be considered to exist, and calling parallel() instead of pool.parallel() will use that?  It's rare for apps to have multiple distinct pools anyway, so this may be an easy way to help obscure a tiny bit more of the implementation.

Rather than making the use of 'break' inside parallel foreach undefined, why not say that it aborts the processing of the current block?

Ideally, the syntax for map() and reduce() should match std.algorithm.  Not sure what this means for blockSize though.  Maybe it should be a template parameter?


Regarding the code...

There are a lot of places where you do:

    lock();
    doSomething();
    unlock();

This isn't exception safe, which isn't a big deal since the enclosed code won't throw anyway, but it's still bad form.  Use this instead:

    synchronized (mutex) {
        doSomething();
    }

There's some black magic in the Mutex class that makes it work with the synchronized statement, so it will do exactly the same thing but be exception-safe as well.  You can even do:

    class C { this() { mutex = new Mutex(this); } }

to make the mutex the enclosing object's monitor so that "synchronized void func() {}" will lock the mutex.  This may turn out to be necessary when/if shared is applied to the code (which will require core.sync to work properly with shared, etc... something I'm going to take care of shortly).

When waiting on condition variables, there's a remote chance that wait() will return when there isn't actually something to process.  Instead of:

    if (!condition)
        wait();

use:

    while (!condition)
        wait();

I saw the "if" approach in "AbstractTask* pop()"... I think that's the only one.

In "ReturnType spinWait()" you throw "exception" if it exists but don't clear the variable.  Will this be a problem?

That's all I saw in a quick scan of the code.  I'll trust that there are no races and play with it in a bit.