<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/2873/">https://reviewboard.asterisk.org/r/2873/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 24th, 2013, 1:40 p.m. CDT, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/2873/diff/3/?file=46134#file46134line217" style="color: black; font-weight: bold; text-decoration: underline;">/branches/12/main/stasis_message_router.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">150</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">route</span><span class="p">)</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">205</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="k">if</span> <span class="p">(</span><span class="n">find_route</span><span class="p">(</span><span class="n">router</span><span class="p">,</span> <span class="n">message</span><span class="p">,</span> <span class="o">&</span><span class="n">route</span><span class="p">)</span> <span class="o">==</span> <span class="mi">0</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">151</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">route</span><span class="o"><span class="hl">-></span></span><span class="n">callback</span><span class="p">(</span><span class="n">route</span><span class="o"><span class="hl">-></span></span><span class="n">data</span><span class="p">,</span> <span class="n">sub</span><span class="p">,</span> <span class="n">topic</span><span class="p">,</span> <span class="n">message</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">206</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span><span class="n">route</span><span class="p"><span class="hl">.</span></span><span class="n">callback</span><span class="p">(</span><span class="n">route</span><span class="p"><span class="hl">.</span></span><span class="n">data</span><span class="p">,</span> <span class="n">sub</span><span class="p">,</span> <span class="n">topic</span><span class="p">,</span> <span class="n">message</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">152</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">}</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">207</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is it possible for route.callback() to be removed by stasis_message_router_remove() by some other thread? If so you should not change struct stasis_message_route from an ao2 object.</pre>
</blockquote>
<p>On September 24th, 2013, 2:44 p.m. CDT, <b>David Lee</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The route variable is local to router_dispatch(), and it's filled in
by find_route() while the lock is held.
So, no, route.callback() can't be changed by any other thread.</pre>
</blockquote>
<p>On September 24th, 2013, 2:48 p.m. CDT, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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 was thinking more of the contents of the struct stasis_message_route becoming invalid after the route is removed. The message_type member is a ref counted object that could become invalid when the route is removed. The same with the data member becoming invalid after it is removed. Calling the callback with the invalid data pointer would not be good.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah, good catch. The data object isn't AO2 managed, so the issue is
also in the original code.
Actually, it's not really an issue with our current usage. Users of
the message router wait until they receive the
stasis_subscription_final_message() from the router before
deallocating the data object (just like they do with a normal
subscription).
This would be less than ideal if we had a use case which required
dynamically adding/removing routes at times other than router
creating/deletion. But I'd be happy just documenting the expected
usage of the API until we need something more elaborate.</pre>
<br />
<p>- David</p>
<br />
<p>On September 23rd, 2013, 11:29 p.m. CDT, David Lee 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.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By David Lee.</div>
<p style="color: grey;"><i>Updated Sept. 23, 2013, 11:29 p.m.</i></p>
<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;">This patch addresses several performance problems that were found in
the initial performance testing of Asterisk 12.
The Stasis dispatch object was allocated as an AO2 object, even though
it has a very confined lifecycle. This was replaced with a straight
ast_malloc().
The Stasis message router was spending an inordinate amount of time
searching hash tables. In this case, most of our routers had 6 or
fewer routes in them to begin with. This was replaced with an array
that's searched linearly for the route.
We more heavily rely on AO2 objects in Asterisk 12, and the memset()
in ao2_ref() actually became noticeable on the profile. This was
#ifdef'ed to only run when AO2_DEBUG was enabled.
After being misled by an erroneous comment in taskprocessor.c during
profiling, the wrong comment was removed.
</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;">Unit tests pass.
Using the setup below, sipp was run against the 12 branch both with
and without my changes.
==
[test-server]$ cat extensions.conf
[performance]
exten => service,1,Answer
exten => service,n,Wait(1)
exten => service,n,Hangup
[test-server]$ cat sip.conf
[perf](!)
type=peer
qualify=no
disallow=all
allow=g722
allow=ulaw
insecure=invite,port
context=performance
[test-client](perf)
host=test-client
Simple sipp scenario:
[test-client]$ sipp -sn uac -l 100 -r 50 tests-server</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>/branches/12/include/asterisk/stasis_message_router.h <span style="color: grey">(399650)</span></li>
<li>/branches/12/main/astobj2.c <span style="color: grey">(399650)</span></li>
<li>/branches/12/main/stasis.c <span style="color: grey">(399650)</span></li>
<li>/branches/12/main/stasis_message_router.c <span style="color: grey">(399650)</span></li>
<li>/branches/12/main/taskprocessor.c <span style="color: grey">(399650)</span></li>
<li>/branches/12/res/res_pjsip/include/res_pjsip_private.h <span style="color: grey">(399650)</span></li>
<li>/branches/12/tests/test_stasis.c <span style="color: grey">(399650)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2873/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>