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

[David Simcha]

On Tue, Aug 31, 2010 at 3:07 PM, Sean Kelly <sean at invisibleduck.org> wrote:

> 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.
>

I could create a *default* lazily initialized Singleton for the TaskPool (a
few other people I've showed this code to have requested this, too, so maybe
it's a trend), but I absolutely insist on being able to manually initialize
it.  First of all, this gives the user the ability to manually decide how
many threads to use.  Secondly, I've had use cases where all threads in a
pool are blocked waiting for the same really expensive computation to
complete, so I fire up a new pool to do that really expensive computation in
parallel.


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

Eventually I'll define it to do something, but I kept it undefined for now
because I don't want to pin down what it does yet.  Ideally I'd like it to
not only stop the current block, but end the processing of the whole rest of
the parallel foreach loop.  This is more difficult to implement, though.


>
> 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?
>

It does match std.algorithm as far as I can tell, as long as you use the
default block size and allow a buffer to be automatically allocated.  If it
doesn't, let me know, because I meant to make it match.


>
>
> 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();
>    }
>

I'm aware that that's not exception safe, but I did it anyhow because I knew
that the enclosed code wouldn't throw.  The reason it's like this is because
I was experimenting with different locking mechanisms, such as making
everything spinlock-based, at one point.  Unless there are strong
objections, I prefer to leave it like this so that I can easily experiment
with different locking mechanisms in the future.

>
> 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.
>

Thanks, will do.  How does this happen, though?


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

I can't see why it would be a problem.  Also, if for some crazy reason the
user handles the exception and then tries to get at the return value again,
IMHO the correct behavior is to throw it again.  If there's something I'm
missing, please let me know.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20100831/ea59b442/attachment.html>