On Wed, 13 Dec 2017 03:15:11 +0000, IM wrote:

> Being new to D, I probably made many mistakes, or did things in a way where there's a more optimum one to do in D. I'm eager to know how to improve, and would like to know if there is any experienced D developer who is thankfully willing to take a look at it and give me feedback?
> 
> Thanks!

I like this API design.


I would separate each unit test to its own `unittest` block (you have three tests in a single block in `SingleThreadTaskRunner.d`); it doesn't really matter with the integrated runner, but if you later use a runner like unit-threaded[1] or tested[2] each unit test block becomes its own independent test, and they will all be run, even if one or more fails.


If `TaskQueue.Pop` returned the Task rather than used an out parameter,
this (in SingleThreadTaskRunner.RunnerLoop):
```
while (true) {
      Task front;
      mQueue.Pop(front);
      if (!front)
        return;

      front();
    }
```
could become:
```
while (true) {
    auto front = mQueue.Pop();
    front ? front() : return;
}

```
Though that's probably debatable as to which is more readable. I would
avoid `out` parameters on void functions; I would at least return a `bool`
and test that instead (which is what `TryPop` does):
```
while (true) {
    Task front;
    if (!mQueue.Pop(front)) {
        return;
    }
    front();
```

I would probably switch the if block to the affirmative case, but that's personal preference.

`Pop` could call `TryPop` to eliminate code duplication. The same is true for other Try functions.


I'll ignore the style since others have discussed it and it's not what you're looking for, but this does look like C#. Style actually is somewhat- important; if I use your library in a project, it would be nice if API calls look like they belong in my code. That's why everybody recommends at least following the Phobos naming guidelines for published libraries.

I only skimmed other reviews, so this may have been mentioned; in `TaskQueue.d`, a private function is misspelled: "AppednTaskThreadUnsafe" -> "AppendTaskThreadUnsafe".

--Ryan


[1]: http://code.dlang.org/packages/unit-threaded
[2]: http://code.dlang.org/packages/tested

On Thursday, 14 December 2017 at 07:47:58 UTC, Ali Çehreli wrote:
> On 12/13/2017 07:57 PM, IM wrote:
>
> >      - Is this the idiomatic way to define a singleton in D?:
> > 
> https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskSystem.d#L24.
>
> > I got this from Ali's book.
>
> I think you got it from Adam D. Ruppe's book.
>

Ooops, sorry, I thought I got it from your book because I've been reading in it recently. But no, I didn't get it from Adam D. Ruppe's book. Turned out that I actually got it from the initOnce() example in the docs here: https://dlang.org/library/std/concurrency/init_once.html

> David Simcha's DConf 2013 presentation has Low-Lock Singleton Pattern at 28 minute mark here:
>
>   https://www.youtube.com/watch?v=yMNMV9JlkcQ&feature=youtu.be&t=1675
>
> Ali

Thanks for the pointer. I'll check it out!

On Thursday, 14 December 2017 at 12:51:07 UTC, rjframe wrote:
> On Wed, 13 Dec 2017 03:15:11 +0000, IM wrote:
>
>> [...]
>
> I like this API design.
>
>
> I would separate each unit test to its own `unittest` block (you have three tests in a single block in `SingleThreadTaskRunner.d`); it doesn't really matter with the integrated runner, but if you later use a runner like unit-threaded[1] or tested[2] each unit test block becomes its own independent test, and they will all be run, even if one or more fails.
>

Interesting. I haven't decided which tester runner to use yet, because the built in one is minimal and is getting me going. But thanks for the pointers. `tested` seems interesting because it's minimal and I like minimal things, however it seems it hasn't been touched for 2 years.

>
> If `TaskQueue.Pop` returned the Task rather than used an out parameter,
> this (in SingleThreadTaskRunner.RunnerLoop):
> ```
> while (true) {
>       Task front;
>       mQueue.Pop(front);
>       if (!front)
>         return;
>
>       front();
>     }
> ```
> could become:
> ```
> while (true) {
>     auto front = mQueue.Pop();
>     front ? front() : return;
> }
>
> ```
> Though that's probably debatable as to which is more readable. I would
> avoid `out` parameters on void functions; I would at least return a `bool`
> and test that instead (which is what `TryPop` does):
> ```
> while (true) {
>     Task front;
>     if (!mQueue.Pop(front)) {
>         return;
>     }
>     front();
> ```
>
> I would probably switch the if block to the affirmative case, but that's personal preference.
>

I like that. I wouldn't switch it to affirmative because I like the early return and not having to add an `else`.

> `Pop` could call `TryPop` to eliminate code duplication. The same is true for other Try functions.
>
>
> I'll ignore the style since others have discussed it and it's not what you're looking for, but this does look like C#. Style actually is somewhat- important; if I use your library in a project, it would be nice if API calls look like they belong in my code. That's why everybody recommends at least following the Phobos naming guidelines for published libraries.
>

That's a fair point, and I agree. I will consider switching to the Phobos
guidelines at some point.

> I only skimmed other reviews, so this may have been mentioned; in `TaskQueue.d`, a private function is misspelled: "AppednTaskThreadUnsafe" -> "AppendTaskThreadUnsafe".
>

Oops, thanks!

> --Ryan
>
>
> [1]: http://code.dlang.org/packages/unit-threaded
> [2]: http://code.dlang.org/packages/tested

I'm still looking for feedback about these points:

  - Any candidate class that can be better switched to a struct or even a template?
  - The use of shared and __gshared, did I get those right? I find the TLS concept unpleasant, because it makes me unsure what's the right thing to do. For example:
    - If a class that instances of which will be accessed by multiple threads, should it be marked `shared`. For instance `TaskQueue` (https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskQueue.d#L9).
    - What about the members of a class, do they ever need to be marked as shared?
    - Also, in this unittest : https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/SingleThreadTaskRunner.d#L148, I didn't mark `number` as shared, even though it is accessed by two threads, and I didn't see any unexpected behavior (because the task runners implicitly synchronize access to it using tasks and replies). But in the other unittest here: https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/ThreadPoolTaskRunner.d#L100, I marked `number` as shared just because I believed I have to since it will be accessed by many threads arbitrarily.

On 15/12/2017 8:15 AM, IM wrote:
> I'm still looking for feedback about these points:
> 
>    - Any candidate class that can be better switched to a struct or even a template?

Template(d/s) symbols are only useful for making things more generic for the purpose of code reuse.

Next it comes to two things regarding class/struct usage.

1) Do you really need the inheritance capabilities with vtables?
2) Is it being passed by value versus reference?

If you don't need inheritance but do need to pass by reference a final class may be appropriate.

If you don't need to copy it around at all, use neither (remember what I said about singletons?).

The purpose of all of this is to make it cheaper at runtime, performance. If you want to write rubbish code while getting the job done (which is not a wrong way of doing things), then ignore all of this and model it as you please.

>    - The use of shared and __gshared, did I get those right? I find the TLS concept unpleasant, because it makes me unsure what's the right thing to do. For example:
>      - If a class that instances of which will be accessed by multiple threads, should it be marked `shared`. For instance `TaskQueue` (https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskQueue.d#L9). 
> 
>      - What about the members of a class, do they ever need to be marked as shared?
>      - Also, in this unittest : https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/SingleThreadTaskRunner.d#L148, I didn't mark `number` as shared, even though it is accessed by two threads, and I didn't see any unexpected behavior (because the task runners implicitly synchronize access to it using tasks and replies). But in the other unittest here: https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/ThreadPoolTaskRunner.d#L100, I marked `number` as shared just because I believed I have to since it will be accessed by many threads arbitrarily.

shared is little more than a way to tell the programmer what your intention is for a give bit of memory (with type safety but not codegen based safety).

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.

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.

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.

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.

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.

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.