[asterisk-dev] [Code Review] Move event cache updates into event processing thread.

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2066/#review6845
-----------------------------------------------------------

Ship it!


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.

- Mark


On July 29, 2012, 7:23 p.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2066/
> -----------------------------------------------------------
> 
> (Updated July 29, 2012, 7:23 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> 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. 
> 
> 
> Diffs
> -----
> 
>   /trunk/main/event.c 370535 
> 
> Diff: https://reviewboard.asterisk.org/r/2066/diff
> 
> 
> Testing
> -------
> 
> Many thousands of calls (on Asterisk 1.8, but the code is pretty much the same in this area)
> 
> 
> Thanks,
> 
> Russell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120731/cd9c1cc5/attachment-0001.htm>