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

Mark Michelson reviewboard at asterisk.org
Thu Jan 10 14:59:45 CST 2013



> On Jan. 10, 2013, 1 p.m., David Lee wrote:
> > /trunk/include/asterisk/taskprocessor.h, line 192
> > <https://reviewboard.asterisk.org/r/2242/diff/2/?file=32638#file32638line192>
> >
> >     The whole named-task-processor gives this function weird semantics. What happens if the taskprocessor already exists? What if it already had a listener?

Good points. Personally, I find it a bit odd that the taskprocessors are all stored in a container in the first place, but I didn't want to rock the boat too badly here. I'll modify this to ensure that the taskprocessor does not currently exist. Obviously, if we restrict this to only being able to create new taskprocessors, then it can't already have a listener :)


> On Jan. 10, 2013, 1 p.m., David Lee wrote:
> > /trunk/include/asterisk/taskprocessor.h, line 225
> > <https://reviewboard.asterisk.org/r/2242/diff/2/?file=32638#file32638line225>
> >
> >     It seems really odd to make this a part of the taskprocessor's API. Doesn't it already have a thread that's popping tasks and executing them?

That's how taskprocessors originally worked. Now there is no inherent threadedness to taskprocessors. Instead, taskprocessors simply notify a listener that tasks have been pushed or that it has become empty. The listeners are free to do what they want. They could execute the tasks in the same thread where they were pushed, they could execute them in a single dedicated thread, or they can farm the tasks off to multiple threads. The original behavior is preserved in the default taskprocessor listener.  However, if using a custom listener, it will need a way to execute tasks in the taskprocessor.  See threadpool_execute() in threadpool.c


> On Jan. 10, 2013, 1 p.m., David Lee wrote:
> > /trunk/include/asterisk/threadpool.h, line 128
> > <https://reviewboard.asterisk.org/r/2242/diff/2/?file=32639#file32639line128>
> >
> >     Can listener be NULL?

After my last set of changes, it can be. I'll update the doxygen to note that the listener, as well as all of the callbacks, is optional.


> On Jan. 10, 2013, 1 p.m., David Lee wrote:
> > /trunk/include/asterisk/threadpool.h, line 133
> > <https://reviewboard.asterisk.org/r/2242/diff/2/?file=32639#file32639line133>
> >
> >     Why are some options arguments to this function, and others fields in the options struct? I recommend putting everything in the options struct.

Good question, and it seems pretty silly to me to have made it this way now. I guess the "why" would be that I initially only had the initial_size argument and added the other options later. I didn't think to add the initial_size into the options for whatever reason.


> On Jan. 10, 2013, 1 p.m., David Lee wrote:
> > /trunk/include/asterisk/threadpool.h, line 78
> > <https://reviewboard.asterisk.org/r/2242/diff/2/?file=32639#file32639line78>
> >
> >     My comments about making taskprocessor_listener an opaque struct apply equally to threadpool_listener.

My response is similar, except here the threapdool listener is already opaque, so there's no need to go down that road. The rest of it, though, I still feel the same way about.


> On Jan. 10, 2013, 1 p.m., David Lee wrote:
> > /trunk/include/asterisk/taskprocessor.h, line 143
> > <https://reviewboard.asterisk.org/r/2242/diff/2/?file=32638#file32638line143>
> >
> >     I think I see what you're doing, and I think you're exposing too much through the API.
> >      * Make ast_taskprocessor_listener and opaque struct
> >        * add accessor functions if you need any
> >      * get rid of the private_data alloc and free callbacks; just put that data in the taskprocessor struct

I had gone down the road of making taskprocessor listener an opaque struct, but I didn't think it was really worth it when I originally coded this.

I have to disagree with your suggestion of putting the private data in the taskprocessor struct. IMO, the taskprocessor and the listener are two separate entities, and in the case of the two taskprocessor listeners that exist now, the default listener and the threadpool, the private data is intrinsic to the listener, not the taskprocessor. For instance, the default taskprocessor listener runs a thread that executes the tasks in the taskprocessor. Information about the thread and its execution belongs in the listener, not the taskprocessor. I guess you're seeing it more that the listener is a part of the taskprocessor and so having the private data in the taskprocessor makes just as much sense as having it in the listener.

I'd be willing to get rid of the alloc callback potentially. ast_taskprocessor_listener_alloc() could just take another parameter that is the private data. The reason it works the way it does right now is due to some chicken-and-egg logic that exists. The private data needs a reference to the taskprocessor so it can execute tasks, the taskprocessor listener owns the private data, and the taskprocessor has a reference to the taskprocessor listener. My solution was to add an allocation function so that listener private data could be allocated once everything else in the taskprocessor and its listener was set up. If I get rid of the alloc callback, then I can just let the private data get its reference to the taskprocessor callback in the listener's start() callback.


> On Jan. 10, 2013, 1 p.m., David Lee wrote:
> > /trunk/main/threadpool.c, line 66
> > <https://reviewboard.asterisk.org/r/2242/diff/2/?file=32641#file32641line66>
> >
> >     I think rather than using a taskprocessor here, maybe what you really want to do is extract the shared threadpool/taskprocessor code into a third thing.

Can you elaborate a bit? "A third thing" is not very descriptive of what you are trying to convey.


- Mark


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


On Jan. 7, 2013, 4:20 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2242/
> -----------------------------------------------------------
> 
> (Updated Jan. 7, 2013, 4:20 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/main/threadpool.c PRE-CREATION 
>   /trunk/include/asterisk/taskprocessor.h 378146 
>   /trunk/include/asterisk/threadpool.h PRE-CREATION 
>   /trunk/main/taskprocessor.c 378146 
>   /trunk/tests/test_taskprocessor.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/20130110/32728c4b/attachment-0001.htm>


More information about the asterisk-dev mailing list