<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/1377/">https://reviewboard.asterisk.org/r/1377/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 23rd, 2011, 2:13 p.m., <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/1377/diff/1-2/?file=18571#file18571line25699" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/channels/chan_sip.c</a>
<span style="font-weight: normal;">
(Diff revisions 1 - 2)
</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; ">static void check_rtp_timeout(struct sip_pvt *dialog, time_t t)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">25699</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">timeout</span> <span class="o">=</span> <span class="n">ast_rtp_instance_get_timeout</span><span class="p">(</span><span class="n">dialog</span><span class="o">-></span><span class="n">rtp</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">25695</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"> </span><span class="kt"><span class="hl">int</span></span><span class="hl"> </span><span class="n">timeout</span> <span class="o">=</span> <span class="n">ast_rtp_instance_get_timeout</span><span class="p">(</span><span class="n">dialog</span><span class="o">-></span><span class="n">rtp</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">25700</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">holdtimeout</span> <span class="o">=</span> <span class="n">ast_rtp_instance_get_hold_timeout</span><span class="p">(</span><span class="n">dialog</span><span class="o">-></span><span class="n">rtp</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">25696</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"> </span><span class="kt"><span class="hl">int</span></span><span class="hl"> </span><span class="n">hold<span class="hl">_</span>timeout</span> <span class="o">=</span> <span class="n">ast_rtp_instance_get_hold_timeout</span><span class="p">(</span><span class="n">dialog</span><span class="o">-></span><span class="n">rtp</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">25701</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">keepalive</span> <span class="o">=</span> <span class="n">ast_rtp_instance_get_keepalive</span><span class="p">(</span><span class="n">dialog</span><span class="o">-></span><span class="n">rtp</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">25697</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="hl"> </span><span class="kt"><span class="hl">int</span></span><span class="hl"> </span><span class="n">keepalive</span> <span class="o">=</span> <span class="n">ast_rtp_instance_get_keepalive</span><span class="p">(</span><span class="n">dialog</span><span class="o">-></span><span class="n">rtp</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;">The coding guidelines to not allow inline variable declarations. Please separate the declaration from the initialization as you had it in your previous diff.</pre>
</blockquote>
<p>On August 23rd, 2011, 2:26 p.m., <b>Kevin Fleming</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 coding guidelines forbid 'mixed code and declarations', but this sort of usage is fine. The guideline was never intended to forbid this, but rather to keep all variable declarations together at the beginning of the enclosing block. Initialization of a variable with a computed value, even one which includes function calls, is acceptable.</pre>
</blockquote>
<p>On August 23rd, 2011, 2:37 p.m., <b>Rob Gagnon</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;">Good to know, Kevin. Thanks. I purposely did not put these at the top as to avoid the function calls if the conditions prior to their use caused the function to exit.</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;">Ahh... my mistake then. I replied too quickly. If these variable declarations are not at the top of the block, then indeed they do not conform to the coding guidelines. At some point we should probably consider revising the guidelines, but at this point, the variable declarations should be at the top of the block, and they can be initialized later. As Richard posted before, it's perfectly acceptable to leave the variable uninitialized until they are used.</pre>
<br />
<p>- Kevin</p>
<br />
<p>On August 23rd, 2011, 1:56 p.m., Rob Gagnon wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/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, Paul Belanger, rmudgett, and Rob Gagnon.</div>
<div>By Rob Gagnon.</div>
<p style="color: grey;"><i>Updated Aug. 23, 2011, 1:56 p.m.</i></p>
<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;">Copying from original post on Jira:
While reviewing how "rtptimeout" and "rtpholdtimeout" operate in code, I found that the check_rtp_timeout() function could be optimized a little to speed up asterisk performance.
Currently, three integers are fetched from the rtp instance multiple times apiece (causing of course, multiple stack operations)
In the worst-case scenario:
- ast_rtp_instance_get_timeout() is called 4 times.
- ast_rtp_instance_get_hold_timeout() is called 4 times.
- ast_rtp_instance_get_keepalive() is called 3 times.
With this patch, each function will be called only once, thus removing up to 9 stack push/pops during runtime
Description posted on jira would be the same
</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;">Compiled code with patch, ran on test system without any problems. Turned logging way up, and verifiied that calls that were silent/disconnected by accident were still being hung-up on.</pre>
</td>
</tr>
</table>
<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-18319">ASTERISK-18319</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/channels/chan_sip.c <span style="color: grey">(333000)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1377/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>