<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/4193/">https://reviewboard.asterisk.org/r/4193/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/4193/diff/1/?file=69105#file69105line255" style="color: black; font-weight: bold; text-decoration: underline;">/team/mjordan/12-threadpool/main/stasis.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct stasis_subscription {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">254</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="cm">/*! Whether or not a thread pool should be used for subscriptions */</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">255</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="kt">int</span> <span class="n">use_thread_pool</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</div>
<br />
<p>- Mark Michelson</p>
<br />
<p>On November 18th, 2014, 6:40 p.m. UTC, Matt Jordan wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated Nov. 18, 2014, 6:40 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-24533">ASTERISK-24533</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/mjordan/12-threadpool/tests/test_stasis.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/res_xmpp.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/res_stasis_device_state.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/res_pjsip_refer.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/res_pjsip_pubsub.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/res_pjsip_mwi.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/res_jabber.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/parking/parking_bridge_features.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/res/parking/parking_applications.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/main/stasis_message_router.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/main/stasis_channels.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/main/stasis_cache.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/main/stasis.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/main/endpoints.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/include/asterisk/stasis_message_router.h <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/include/asterisk/stasis_internal.h <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/include/asterisk/stasis.h <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/channels/sig_pri.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/channels/chan_skinny.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/channels/chan_sip.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/channels/chan_mgcp.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/channels/chan_iax2.c <span style="color: grey">(428167)</span></li>
<li>/team/mjordan/12-threadpool/channels/chan_dahdi.c <span style="color: grey">(428167)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4193/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>