Is it possible to request a code review?

[IM]

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.