www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Is it possible to request a code review?

reply IM <3di gm.com> writes:
I started learning D recently. As part of the learning process, I 
started working on some D projects to experiment and learn. My 
first project is a tasking system library that I plan to use in 
another D project I'm working on.

The goal is to be able to post and run any task asynchronously on 
current thread, new thread, or thread pool, and possibly getting 
a callback on the posting thread when the task has run to 
completion. I will probably expand it more as I go. I've just 
published it on DUB here: 
https://code.dlang.org/packages/libdtasks

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!
Dec 12 2017
next sibling parent reply bauss <jj_1337 live.dk> writes:
On Wednesday, 13 December 2017 at 03:15:11 UTC, IM wrote:
 Thanks!
First thing. Generally in D module names are kept lower-case. To give an example your: AbstractTaskRunner.d module tasks.AbstractTaskRunner; Would usually be: abstracttaskrunner.d module tasks.abstracttaskrunner; Of course it's not necessary, but it's how it usually is in idiomatic D. The same goes for function names etc. they're usually kept camelCase and not PascalCase like you have it. Again it's not necessary, but that's idiomatic D. A wild guess from me would be that you come from a C#/.NET background OR you have worked heavily with the win api, am I right? Documentation can be done like this for multiline: /** * ... * ... * etc. */ Instead of: /// ... /// ... /// etc. This might just be a personal thing for me, but I always use /// for single-line documentation only. Also you don't need to define constructors for classes, if they don't actually do something. Ex. your SingleThreadTaskRunner class defines this: ~this() safe {} Integers are automatically initialized to int.init which is 0, so the following is can be rewritten from: (Find in a unittest) int value = 0; to: int value; Also empty lambdas like: () { ... } can be written like: { ... } Also you have a few Hungarian notations, which is often not used in idiomatic D. The key for an associative array __should__ always be treated const internally, so this: ITaskRunner[const Tid] shouldn't be necessary and doing: ITaskRunner[Tid] should work just fine. If you have experienced otherwise, then I'd argue it's a bug and you should report it. Also statements like this: immutable int currentNumber are often preferred written like: immutable(int) currentNumber And getting too used to write: immutable int currentNumber might have you end up writing something like: immutable int getAnImmutableNumber() { ... } Which does not do what you would expect as it doesn't treat the return value as immutable, but instead "this" as immutable. To make the return type immutable you'd write: immutable(int) getAnImmutableNumber() { ... } I don't know if you know that already, but just figured I'd give a heads up about that. That was all I could see looking through your code real quick. Take it with a grain of salt as some of the things might be biased.
Dec 12 2017
next sibling parent reply Jonathan M Davis <newsgroup.d jmdavisprog.com> writes:
On Wednesday, December 13, 2017 06:14:04 bauss via Digitalmars-d wrote:
 Documentation can be done like this for multiline:

 /**
 * ...
 * ...
 * etc.
 */

 Instead of:

 /// ...
 /// ...
 /// etc.
You can also do /** ... ... etc. */ or /++ ... ... etc. +/ Personally, I always use the last one if the ddoc comment is multiple lines, and I've never understood why anyone would want all those extra *'s on the side. - Jonathan M Davis
Dec 12 2017
parent IM <3di gm.com> writes:
On Wednesday, 13 December 2017 at 07:30:55 UTC, Jonathan M Davis 
wrote:
 On Wednesday, December 13, 2017 06:14:04 bauss via 
 Digitalmars-d wrote:
 Documentation can be done like this for multiline:

 /**
 * ...
 * ...
 * etc.
 */

 Instead of:

 /// ...
 /// ...
 /// etc.
You can also do /** ... ... etc. */ or /++ ... ... etc. +/ Personally, I always use the last one if the ddoc comment is multiple lines, and I've never understood why anyone would want all those extra *'s on the side. - Jonathan M Davis
I think it is believed that the extra stars: /** * <Insert docs here> * <Insert docs here> */ make the docs stand out and look better (as if they're inside a blockquote or something). However, doc styles and code style in general is not my primary concern right now. I want dive deeply into D first, and worry about style later.
Dec 13 2017
prev sibling parent reply IM <3di gm.com> writes:
On Wednesday, 13 December 2017 at 06:14:04 UTC, bauss wrote:
 On Wednesday, 13 December 2017 at 03:15:11 UTC, IM wrote:
 Thanks!
First thing. Generally in D module names are kept lower-case. To give an example your: AbstractTaskRunner.d module tasks.AbstractTaskRunner; Would usually be: abstracttaskrunner.d module tasks.abstracttaskrunner; Of course it's not necessary, but it's how it usually is in idiomatic D. The same goes for function names etc. they're usually kept camelCase and not PascalCase like you have it. Again it's not necessary, but that's idiomatic D. A wild guess from me would be that you come from a C#/.NET background OR you have worked heavily with the win api, am I right?
No, I'm coming from a C++ background. Though I did a bit of C# long time ago.
 Documentation can be done like this for multiline:

 /**
 * ...
 * ...
 * etc.
 */

 Instead of:

 /// ...
 /// ...
 /// etc.

 This might just be a personal thing for me, but I always use 
 /// for single-line documentation only.

 Also you don't need to define constructors for classes, if they 
 don't actually do something.

 Ex. your SingleThreadTaskRunner class defines this:

 ~this()  safe {}

 Integers are automatically initialized to int.init which is 0, 
 so the following is can be rewritten from: (Find in a unittest)

 int value = 0;

 to:

 int value;
I feel like it doesn't hurt to be explicit about it, but I get your point.
 Also empty lambdas like:

 () { ... }

 can be written like:

 { ... }
That's actually better, thanks!
 Also you have a few Hungarian notations, which is often not 
 used in idiomatic D.

 The key for an associative array __should__ always be treated 
 const internally, so this:

 ITaskRunner[const Tid]

 shouldn't be necessary and doing:

 ITaskRunner[Tid] should work just fine.

 If you have experienced otherwise, then I'd argue it's a bug 
 and you should report it.
I think I made it so, because I had a function with signature: void Foo(const Tid tid) { // Use `tid` to access the associative array, which refused // to compile unless the key was marked `const` in the declaration. }
 Also statements like this:

 immutable int currentNumber

 are often preferred written like:

 immutable(int) currentNumber

 And getting too used to write:

 immutable int currentNumber

 might have you end up writing something like:

 immutable int getAnImmutableNumber() { ... }

 Which does not do what you would expect as it doesn't treat the 
 return value as immutable, but instead "this" as immutable.

 To make the return type immutable you'd write:

 immutable(int) getAnImmutableNumber() { ... }

 I don't know if you know that already, but just figured I'd 
 give a heads up about that.
No, I didn't know. Thanks for mentioning!
 That was all I could see looking through your code real quick.

 Take it with a grain of salt as some of the things might be 
 biased.
Thanks. That was definitely helpful. I was also looking for more feedback about the following: - 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/SingleThre dTaskRunner.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/ThreadPo lTaskRunner.d#L100, I marked `number` as shared just because I believed I have to since it will be accessed by many threads arbitrarily. - Is this the idiomatic way to define a singleton in D?: https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tas s/TaskSystem.d#L24. I got this from Ali's book. Thank you!
Dec 13 2017
next sibling parent reply rikki cattermole <rikki cattermole.co.nz> writes:
On 14/12/2017 3:57 AM, IM wrote:

snip

      - Is this the idiomatic way to define a singleton in D?: 
 https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tas
s/TaskSystem.d#L24. 
You say singleton I think wrong. Use free-functions and globals instead. Singletons are always a code smell that OOP based languages like to say are a 'good thing'. e.g. module taskmgr; import task; private __gshared { Task[] tasks; } Task[] getTasks() { return tasks; } void clearTasks() { tasks = null; } void addTask(Task t) { tasks ~= t; }
Dec 13 2017
parent IM <3di gm.com> writes:
On Thursday, 14 December 2017 at 04:12:33 UTC, rikki cattermole 
wrote:
 On 14/12/2017 3:57 AM, IM wrote:

 snip

      - Is this the idiomatic way to define a singleton in D?: 
 https://gitlab.com/3d_immortal/libdtasks/blob/master/src/tasks/TaskSystem.d#L24.
You say singleton I think wrong. Use free-functions and globals instead. Singletons are always a code smell that OOP based languages like to say are a 'good thing'. e.g. module taskmgr; import task; private __gshared { Task[] tasks; } Task[] getTasks() { return tasks; } void clearTasks() { tasks = null; } void addTask(Task t) { tasks ~= t; }
Sure, that works too, Thanks. However, we could leave the debate about the pros and cons of singletons for another day. That's not what my question was about though.
Dec 13 2017
prev sibling parent reply =?UTF-8?Q?Ali_=c3=87ehreli?= <acehreli yahoo.com> writes:
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/tas s/TaskSystem.d#L24.
 I got this from Ali's book.
I think you got it from Adam D. Ruppe's book. 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
Dec 13 2017
parent IM <3di gm.com> writes:
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!
Dec 14 2017
prev sibling next sibling parent reply =?UTF-8?Q?Ali_=c3=87ehreli?= <acehreli yahoo.com> writes:
On 12/12/2017 07:15 PM, IM wrote:
 I started learning D recently.
Welcome! There is also the Learn newsgroup[1]. ;) Ali [1] Available through a forum interface here: http://forum.dlang.org/group/learn
Dec 12 2017
parent IM <3di gm.com> writes:
On Wednesday, 13 December 2017 at 07:02:47 UTC, Ali Çehreli wrote:
 On 12/12/2017 07:15 PM, IM wrote:
 I started learning D recently.
Welcome! There is also the Learn newsgroup[1]. ;) Ali [1] Available through a forum interface here: http://forum.dlang.org/group/learn
Thanks, I posted a question there once already.
Dec 13 2017
prev sibling next sibling parent reply rjframe <dlang ryanjframe.com> writes:
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
Dec 14 2017
parent reply IM <3di gm.com> writes:
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
Dec 15 2017
parent reply IM <3di gm.com> writes:
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/SingleThre
dTaskRunner.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/ThreadPo
lTaskRunner.d#L100, I marked `number` as shared just because I believed I have
to since it will be accessed by many threads arbitrarily.
Dec 15 2017
parent rikki cattermole <rikki cattermole.co.nz> writes:
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/ta
ks/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/SingleThre
dTaskRunner.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/ThreadPo
lTaskRunner.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).
Dec 15 2017
prev sibling parent reply Neia Neutuladh <neia ikeran.org> writes:
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.
Dec 15 2017
parent IM <3di gm.com> writes:
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.
Dec 15 2017