<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/2066/">https://reviewboard.asterisk.org/r/2066/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<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've been mulling this over in my head a bit. At first, I took issue to the fact that, with this change, consumers of the event cache may wind up reading inaccurate data. For instance, a device state change may not result in the cache being updated until after other device state updates have been processed. If a consumer attempts to read the device state during that gap, then the consumer will get an out-of-date device state.
Then I thought a bit more, and the current system is odd, too. A consumer could potentially read a cached device state and then be notified of that device state change. That seems a bit out or order to me.
I think that the occurrence of the event, meaning when the event is handled by the event system, should be the time when the event is cached. Given that, this patch seems fine to me. Since this isn't fixing a reported bug, my inclination is to put this in trunk only. I can be persuaded otherwise if people report that this has both improved performance significantly AND not caused any new issues.</pre>
<br />
<p>- Mark</p>
<br />
<p>On July 29th, 2012, 7:23 p.m., Russell Bryant 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 Russell Bryant.</div>
<p style="color: grey;"><i>Updated July 29, 2012, 7:23 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;">Prior to this patch, updating the device state cache was done by the thread that originated the event. It would update the cache and then queue the event up for another thread to dispatch. This thread moves the cache updating part to be in the same thread as event dispatching.
I was working with someone on a heavily loaded Asterisk system and while reviewing backtraces of the system while it was having problems, I noticed that there were a lot of threads contending for the lock on the event cache. By simply moving this into a single thread, this helped performance *a lot* and alleviated some deadlock-like symptoms.
This patch was important to this system as it helps fix some issues that were problems there. I would like to put it in Asterisk 1.8, but it's not really an obvious candidate. In addition to code review, I'd like some opinions on which branch this can go in. </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;">Many thousands of calls (on Asterisk 1.8, but the code is pretty much the same in this area)</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>/trunk/main/event.c <span style="color: grey">(370535)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2066/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>