<br><br><div class="gmail_quote">On Tue, Aug 31, 2010 at 3:07 PM, Sean Kelly <span dir="ltr"><<a href="mailto:sean@invisibleduck.org">sean@invisibleduck.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
A few general comments:<br>
<br>
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.<br>
</blockquote><div><br>I could create a <b>default</b> 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.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Rather than making the use of 'break' inside parallel foreach undefined, why not say that it aborts the processing of the current block?<br></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
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?<br></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
<br>
Regarding the code...<br>
<br>
There are a lot of places where you do:<br>
<br>
lock();<br>
doSomething();<br>
unlock();<br>
<br>
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:<br>
<br>
synchronized (mutex) {<br>
doSomething();<br>
}<br></blockquote><div><br>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. <br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
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:<br>
<br>
class C { this() { mutex = new Mutex(this); } }<br>
<br>
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).<br>
<br>
When waiting on condition variables, there's a remote chance that wait() will return when there isn't actually something to process. Instead of:<br>
<br>
if (!condition)<br>
wait();<br>
<br>
use:<br>
<br>
while (!condition)<br>
wait();<br>
<br>
I saw the "if" approach in "AbstractTask* pop()"... I think that's the only one.<br></blockquote><div><br>Thanks, will do. How does this happen, though?<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
In "ReturnType spinWait()" you throw "exception" if it exists but don't clear the variable. Will this be a problem?<br></blockquote><div><br>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.<br>
<br></div></div><br>