[asterisk-dev] [Code Review] 4193: Stasis: allow for subscriptions to dictate message delivery on a threadpool for certain situations

[Mark Michelson]

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


I think updating the setup_stasis_subs() function in app_queue.c to use stasis_message_router_create_pool() would be appropriate. Those message routers are set up for each call into the queue application, only care about a few message types that can happen in the bridge, and are destroyed when the call is completed. For systems with heavy queue use, this could result in a large number of threads being created/destroyed.


/team/mjordan/12-threadpool/main/stasis.c
<https://reviewboard.asterisk.org/r/4193/#comment24307>

    I don't see the use for this field on the structure. The only place it is used is in internal_stasis_subscribe(), and in that context, the use_thread_pool function parameter could be used instead.


- Mark Michelson


On Nov. 18, 2014, 6:40 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4193/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2014, 6:40 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24533
>     https://issues.asterisk.org/jira/browse/ASTERISK-24533
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Note: don't be fooled by the size of this review. Most of the changes are small and are in stasis.c/stasis_message_router.c.
> 
> Rob (and CDR on the asterisk-users list, although he never followed up when we asked for more information) discovered that we were creating two additional threads per SIP peer in chan_sip. This actually would occur for a large variety of 'endpoints' in Asterisk, regardless of the channel driver. The two threads were stasis subscriptions, specifically for MWI and for endpoints detecting the destruction of related channels.
> 
> Stasis subscriptions currently get a dedicated thread. In contrast, prior to r400178 (see review https://reviewboard.asterisk.org/r/2881/), the subscriptions shared a thread pool. It was discovered that for a low subscription count with high message throughput, the threadpool was not as performant as simply having a dedicated thread per subscriber.
> 
> Since then, our subscription pattern has changed slightly. While we still have plenty of subscriptions that would follow the model just described (AMI, CDRs, CEL, etc.) - there are plenty that also fall into the following two categories:
> * Large number of subscriptions, specifically those tied to endpoints/peers.
> * Low number of messages. Some subscriptions exist specifically to coordinate a single message - the subscription is created, a message is published, the delivery is synchronized, and the subscription is destroyed.
> In both of the latter two cases, creating a dedicated thread is wasteful (and in the case of a large number of peers/endpoints, harmful).
> 
> This patch adds the ability of a subscriber to Stasis to choose whether or not their messages are dispatched on a dedicated thread or on a threadpool. The threadpool is currently hard coded in this patch; if configuration is necessary, we could re-add the configuration options in r2881. Pre-mature optimization and all that led me to simply go with a simpler model for now.
> 
> Note that the approach taken here was to add additional API calls rather than modify existing ones. If we'd rather the thread dispatch model be dictated by a parameter, the new API calls can be removed in trunk and the old API calls modified appropriately.
> 
> 
> Diffs
> -----
> 
>   /team/mjordan/12-threadpool/tests/test_stasis.c 428167 
>   /team/mjordan/12-threadpool/res/res_xmpp.c 428167 
>   /team/mjordan/12-threadpool/res/res_stasis_device_state.c 428167 
>   /team/mjordan/12-threadpool/res/res_pjsip_refer.c 428167 
>   /team/mjordan/12-threadpool/res/res_pjsip_pubsub.c 428167 
>   /team/mjordan/12-threadpool/res/res_pjsip_mwi.c 428167 
>   /team/mjordan/12-threadpool/res/res_jabber.c 428167 
>   /team/mjordan/12-threadpool/res/parking/parking_bridge_features.c 428167 
>   /team/mjordan/12-threadpool/res/parking/parking_applications.c 428167 
>   /team/mjordan/12-threadpool/main/stasis_message_router.c 428167 
>   /team/mjordan/12-threadpool/main/stasis_channels.c 428167 
>   /team/mjordan/12-threadpool/main/stasis_cache.c 428167 
>   /team/mjordan/12-threadpool/main/stasis.c 428167 
>   /team/mjordan/12-threadpool/main/endpoints.c 428167 
>   /team/mjordan/12-threadpool/include/asterisk/stasis_message_router.h 428167 
>   /team/mjordan/12-threadpool/include/asterisk/stasis_internal.h 428167 
>   /team/mjordan/12-threadpool/include/asterisk/stasis.h 428167 
>   /team/mjordan/12-threadpool/channels/sig_pri.c 428167 
>   /team/mjordan/12-threadpool/channels/chan_skinny.c 428167 
>   /team/mjordan/12-threadpool/channels/chan_sip.c 428167 
>   /team/mjordan/12-threadpool/channels/chan_mgcp.c 428167 
>   /team/mjordan/12-threadpool/channels/chan_iax2.c 428167 
>   /team/mjordan/12-threadpool/channels/chan_dahdi.c 428167 
> 
> Diff: https://reviewboard.asterisk.org/r/4193/diff/
> 
> 
> Testing
> -------
> 
> New unit tests were written to cover the new subscription types. This tests receiving messages sent using the threadpool.
> 
> The MWI tests and channel driver tests in the Test Suite were run and passed.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141120/bd1136c7/attachment-0001.html>