[asterisk-dev] [Code Review]: Threadpool Support

[Mark Michelson]

> On Dec. 27, 2012, 9:55 a.m., Matt Jordan wrote:
> > /trunk/include/asterisk/threadpool.h, line 27
> > <https://reviewboard.asterisk.org/r/2242/diff/1/?file=32359#file32359line27>
> >
> >     I have a feeling that most consumers of a thread pool will be okay with a default listener. I can see the typical use being:
> >     
> >     1) Make me a thread pool
> >     2) I push tasks onto the thread pool with the callbacks I define
> >     3) When I'm done, I want the thread pool to shut down
> >     
> >     Often, synchronization of and/or knowledge of the thread pool's worker threads isn't really needed - it's good that you can create your own listener to get that info, but I don't think all consumers will need that level of granularity.
> >     
> >     On that same note, I may want to provide a listener that only gets a subset of these notifications - which means it'd be nice if some of these callbacks could be NULL. That would mean the code in the threadpool would have to be updated to check for the existence of the callbacks before executing them.
> >     
> >     If we don't want to provide a generic listener, I'd make the threadpool listener optional, i.e., allow it to be NULL.

Okay, I'll make the listener optional as well as all of its callbacks.

The most common operation that a listener would want to do is to change the threadpool size based on activity. Of course, I've added options so that a threadpool can automatically add threads when there are currently no idle threads and I've added an option so that idle threads can time out. With these two options set, most threadpools will probably not need much intervention. However, the option to have a listener can be useful for specific scenarios, potentially. That being said, there's not a real strong case for having a generic threadpool listener, which is why I'm just going with making the listener and its specific callbacks optional.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2242/#review7580
-----------------------------------------------------------


On Dec. 11, 2012, 12:53 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2242/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2012, 12:53 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This review adds a generic threadpool for Asterisk.
> 
> The threadpool implementation here is very similar to the one implemented in Asterisk SCF. Since this is implemented in C, the "is a" semantics offered by C++ are not available, so that had to be worked around.
> 
> The threadpool, when created, creates a taskprocessor. The threadpool itself exists as the private data on a taskprocessor listener. In this way, the threadpool can react to changes on the taskprocessor by informing its threadpool listener.
> 
> The threadpool informs its listeners of various changes:
> - When a task gets pushed into the threadpool
> - When the threadpool's task list has become empty
> - When the state of the threadpool's threads has changed, such as when an active thread goes idle or an idle thread is destroyed.
> 
> The threadpool listener can react to these changes as it sees fit. This allows for different policies to be adopted by different modules.
> 
> The offers some options for automatic behavior for common forms of operation. At allocation, an idle timeout can be specified in order to allow for idle threads to automatically get removed from the threadpool once they have been idle for a certain amount of time. Also, an automatic increment can be specified if the threadpool has tasks added to it and no idle threads are available to handle the task. With these, it may be possible for listeners only to intervene in certain situations. More options can possibly be added if they are not too policy-specific.
> 
> This set of changes is dependent on the set of taskprocessor changes introduced in https://reviewboard.asterisk.org/r/2200 . Since these changes were developed in the same branch as the taskprocessor changes, it means that all the taskprocessor changes are also included in this review. While there are minor changes in the taskprocessor code here as compared to the code in review 2200, they are very minor changes, and so close scrutiny of the taskprocessor changes is not as important as the threadpool code itself.
> 
> 
> This addresses bug ASTERISK-20691.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20691
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/taskprocessor.h 377805 
>   /trunk/include/asterisk/threadpool.h PRE-CREATION 
>   /trunk/main/taskprocessor.c 377805 
>   /trunk/main/threadpool.c PRE-CREATION 
>   /trunk/tests/test_threadpool.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2242/diff
> 
> 
> Testing
> -------
> 
> A suite of unit tests have been added to ensure that the threadpool works as expected. They all pass.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130107/a1f0634c/attachment.htm>