Add a ThreadPool class to the threads package. #490
Conversation
|
I have't gotten a chance to review this yet. However, it is good to have separate commits for tests and documentation updates. This PR can be split to 3 commits. |
Any commit that adds something new can be split up into "Add a thing", "Document the thing" and "Test the thing", but it is completely unclear to me why that is better. Splittting it up means that individual commits can be reverted, but why would you revert the addition without also reverting the testing or documentation? In short, I don't understand the benefits of separating documentation and testing from the addition of the thing itself. |
|
Any commit that can be split to more logical commits, not only tests/docs, should be spilt to more logical commits. It means that the author did put some effort into the design of the feature and organizing the code into small modular additions. It also makes it easier to review the code because the reviewer can follow those smaller commits. In exactly 3 years from now we will discover a bug in this thread pool and we trace it back to this commit, it is easier to look at the actual code changes and not have to swift through 100s of other changes that include docs/tests. I can keep going with more reasons why spiting changes into smaller commits is a good idea. |
uni/lib/thread.icn
Outdated
| #-------------------------------------------------------------------------------- | ||
| # Construct a pool of n worker threads with default allocations. | ||
| # Subsequent calls add workers to the pool | ||
| method MakePool(n: integer: 0) | ||
| if n <= 0 then n := default_pool_size() | ||
| # Create the requested number of workers and tell them the class instance | ||
| critical Work: { | ||
| while 0 <= (n-:= 1) do { ::put(Workforce, thread classworker(self)) } | ||
| } | ||
| return self #success | ||
| end | ||
|
|
||
| #-------------------------------------------------------------------------------- | ||
| # Construct a pool of n worker threads with specified stack, string and block allocations | ||
| # Subsequent calls add workers to the pool | ||
| method MakePoolX(n: integer: 0, blkSp, strSp, stkSp) |
There was a problem hiding this comment.
Why define two function doing the same thing more or less? MakePoolX() can handle everything MakePool() can, plus more. If you drop the initial allocation size per the class, and leave it to MakePoolX() to initialize those.
I suggest making it simpler: rename MakePooXl() to MakePool(). Simpler class is easier to use, and we don't lose any functionality.
uni/lib/thread.icn
Outdated
| # Create the requested number of workers | ||
| critical Work: { | ||
| if /strSp & /blkSp & /stkSp then { # Use default allocation | ||
| while 0 <= (n-:= 1) do { ::put(Workforce, thread classworker(self)) } |
There was a problem hiding this comment.
I've stared at 0 <= (n-:= 1) for a bit because I expected it to do more than just a loop counter that is not used. I think this line should just be:
every !n do ::put(Workforce, thread classworker(self))
!n or 1 to n makes it clear "I want to do this n times and I don't care about the counter", the loop doesn't care about the value n. No need for braces either.
uni/lib/thread.icn
Outdated
| while 0 <= (n-:= 1) do { | ||
| ::put(Workforce,::spawn(create(classworker(self)),blkSp, strSp, stkSp)) | ||
| } |
uni/lib/thread.icn
Outdated
| local n | ||
| critical Work: n := *Workforce - Idlers | ||
| return n |
There was a problem hiding this comment.
The lock here isn't useful. return *Workforce - Idlers is just as good without the lock.
uni/lib/thread.icn
Outdated
| # Add a task (described by a procedure plus parameters) to the end of the list of tasks | ||
| # to be executed by (one of) the pool of worker threads. | ||
| method Dispatch(p, args[]) | ||
| if ::type(p) == "procedure" then { |
There was a problem hiding this comment.
Why not support methods too ? I use threads with methods often. I suggest passing the instance and the method to be invoked , versus supporting procedures only.
| method IsIdle() | ||
| ::lock(Work) | ||
| if ((Idlers ~= *\Workforce) | (*ToDo > 0)) then { ::unlock(Work); fail } | ||
|
|
||
| ::unlock(Work) | ||
| return # success | ||
| end |
There was a problem hiding this comment.
Practically the same as (no need to lock)
(Idlers = *\Workforce) & (*ToDo = 0) & return
uni/lib/thread.icn
Outdated
| # will be executed before closure. Fails if all work has not been done. | ||
| method ClosePool() | ||
| local n | ||
| every n := 1 to *Workforce do Dispatch(stopWork) |
There was a problem hiding this comment.
every !*Workforce do Dispatch(stopWork)
uni/lib/thread.icn
Outdated
| every ::wait(!Workforce) | ||
|
|
||
| # At this point, Idlers must be zero. | ||
| if Idlers > 0 then ::stop("(ClosePool) Idlers queue not empty") |
There was a problem hiding this comment.
library functions shouldn't exit, generally speaking.
If the change is large, or can be split up into functionally separate, modules that can exist more or less independently then I agree it is better (and clearer) to split it up into separate commits. However, I disagree with the idea we should encourage the separation of testing and documentation from the code itself. I don't think the test code (or the documentation) are separate modules; and all that splitting them into separate commits does is encourage the attitude of concentrating on just the code with "I'll get round to adding tests and documenting it later" -- later often doesn't happen. If we have to come back to a particular commit after a couple of years to fix it, we can readily see if the tests and/or documentation also need amemdment. Whereas, if they are separated, we have to wonder if they exist, find the relevant commits and then see if they require alteration (and we have to remember to do that, not just fix the code). So for larger changes we are in agreement but for smaller changes, such as this one, we have opposing opinions. |
|
Separating test and documentation doesn't mean you can't do them as you write the actual code. You do them all at the same time. When you commit, you just group code together, testing together, documentation together. The code change commit comment describes the code change. Then the test commit does the same, followed by documentation. It is all one PR. If I look at the PR I can see the that you did all three. I can review each stage separately. You never have to wonder about what changed when you use git. When I edit a line in my editor, I know who changed the line, and when, and in what PR. with a hover I know what commits exist in the PR and if a test and/or documentation came with that code change on separate commits even before I have to browse the actual changes of the commits. I can jump to the documentation commit right away if it is there. In all cases, this is just a suggestion given the lessons I learned from my experience working on several projects. Feel free to ignore if you don't see the value as I see it. |
Don-Ward
force-pushed
the
ThreadPoolClass
branch
2 times, most recently
from
4ea615c to
02c38f7
Compare
The new class is documented in Appendix C of the Unicon book. Tests have been added to tests/threads to test priority queuing and the queuing of methods.
Don-Ward
force-pushed
the
ThreadPoolClass
branch
from
02c38f7 to
6b1ff1e
Compare
The new class is documented in Appendix C of the Unicon book. A test has been added to tests/threads.