On Friday, 15 December 2017 at 21:34:48 UTC, Neia Neutuladh wrote:
On Wednesday, 13 December 2017 at 03:15:11 UTC, IM wrote:
https://code.dlang.org/packages/libdtasks
I'd get rid of the ITaskRunner interface and rename
AbstractTaskRunner to TaskRunner. Interfaces are fine when you
need them, but you rarely need them. You can just make
PostTaskAndReply non-final and it's the same. It's slightly
cheaper than calling the final method through an interface,
even.
That's a fair point. Thanks.
I'd also eliminate TaskSystem. You can use a thread-local
TaskRunner pretty much the same way, and it's a bit weird to
have policy based on thread ID. (Normally, you would have a
threadpool-like thing for each type of task. Like one for
serving web traffic, one for transcoding video, one for reading
RSS feeds, etc.) Thread IDs can be reused, so if you forget to
clean something up, you might end up reusing the wrong
TaskRunner later. A thread-local variable also reduces the work
you have to do to stop a thread.
In any case, the TaskRunner-per-thread thing is a policy
decision that's not really appropriate for a library to make.
You generally try to leave that to applications.
Applications can instantiate multiple single thread task runners,
each for a specific type of tasks as you mentioned earlier. They
can also instantiate a thread pool task runners for tasks that
are independent, don't need to run sequentially or any order, or
on any particular thread.
TaskQueue has a race condition, I think:
Thread 1 calls Pop(). It acquires the mutex.
Thread 1 observes there are no tasks available to pop. It
waits on the condition variable.
Thread 2 calls Push(). It tries to acquire the mutex, but the
mutex is taken. It blocks forever.
That's not true. Waiting on a condition variable should unlocked
the mutex first, the put the thread to sleep. That's how most
implementations are. See for example:
http://en.cppreference.com/w/cpp/thread/condition_variable/wait
Reading code is generally easier when that code follows the
style guide for the language. https://dlang.org/dstyle.html and
maybe run dfmt on your code. For instance, when you write
`atomicOp !"+="`, it takes me about a full second to connect
that to the common style of `atomicOp!"+="`. If I worked with
your code for an hour, that would go away, but it's a bit of a
speedbump for new contributors.
Yeah, if you noticed, I use clang-format rather than dfmt. I
found dfmt to be very buggy, especially when using 80-character
line limit. It also produces formatted code that I don't like,
especially when there's multiple indentations. clang-format
doesn't support D unfortunately, so I have to add '//
clang-format off .. on' here and there. It is clang-format that
added that extra space after 'atomicOp!'. I wish it has full
support for D like it does for Java and Javascript, but hey it
mostly works, so fine.
One other point of note about formatting: I never regret using
braces around single-statement blocks. Like:
while (!done && tasks.length == 0)
{
condition.wait;
}
I just so frequently need to add logging or extra bookkeeping
to statements like that that it's not even worth the time to
try to omit braces.
I believe less braces and less indentation is better. I like to
keep the code minimal with minimal whitespace around it, but
that's just my personal pereferance.
Thank you very much for your review.
