<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/2242/">https://reviewboard.asterisk.org/r/2242/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 16th, 2013, 2:44 p.m., <b>David Lee</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/2242/diff/3/?file=32831#file32831line157" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/include/asterisk/threadpool.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void ast_threadpool_set_size(struct ast_threadpool *threadpool, unsigned int size);</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">157</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * \param task The task to add</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;">What's the significance of the int returned by the task?</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;">It has none. Changing it not to return int means having to change every user of taskprocessors to use void-returning functions.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 16th, 2013, 2:44 p.m., <b>David Lee</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/2242/diff/3/?file=32835#file32835line552" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/tests/test_threadpool.c</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">AST_TEST_DEFINE(threadpool_thread_timeout)</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">552</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">res</span> <span class="o">=</span> <span class="n">wait_until_thread_state</span><span class="p">(</span><span class="n">test</span><span class="p">,</span> <span class="n">tld</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="mi">1</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;">It's probably worth a note that timing-related tests such as these can be brittle on heavily loaded systems. For example, a build machine running several builds in parallel may fail this test, simply because a thread was starved and didn't receive its update in a timely fashion.</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;">Yep, I tried to put the timeouts much much higher than you'd actually need them to be so that even if a system were loaded, there is enough tolerance to allow for the load. Of course, I felt that I had to put the timeouts in because if there were some sort of issue with the threadpools such that they are not updated as expected, then the test would hang forever.</pre>
<br />
<p>- Mark</p>
<br />
<p>On January 15th, 2013, 3:28 p.m., Mark Michelson 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.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated Jan. 15, 2013, 3:28 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;">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.</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;">A suite of unit tests have been added to ensure that the threadpool works as expected. They all pass.</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-20691">ASTERISK-20691</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/main/taskprocessor.c <span style="color: grey">(379127)</span></li>
<li>/trunk/include/asterisk/threadpool.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/include/asterisk/taskprocessor.h <span style="color: grey">(379127)</span></li>
<li>/trunk/main/threadpool.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/tests/test_taskprocessor.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/tests/test_threadpool.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2242/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>