[asterisk-dev] [Code Review] Hints and devices from hints moved to ao2_container

[Tilghman Lesher]

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



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6113>

    Add 'static' prefix.



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6114>

    The comment here says CMP_STOP is not returned, yet it clearly is.



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6115>

    The one place that references this function does not check the return value, so this should return void, not void *.  Additionally, I am uncomfortable with a function that decrements the refcount of an object twice (remember, the unlink decrements the reference implicitly).  Any additional unref should be in the parent function, where the device is originally referenced.  In fact, I'm suspicious that this is, in fact, a bug.



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6116>

    Making extenhint_destroy into a separate function is just silly, given that it's a single line of code, and is not referenced from anywhere else.



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6118>

    Looks like you've forgotten to decrement the working refcount to device, here.



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6117>

    Flip these two statements.  You never want to reference an object after you've decremented your own refcount.



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6119>

    Missing a refcount deref.



trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/1003/#comment6120>

    Move the deref until AFTER your last usage of it.


- Tilghman


On 2010-11-08 08:38:59, schmidts wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1003/
> -----------------------------------------------------------
> 
> (Updated 2010-11-08 08:38:59)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> handle_statechange in pbx.c has a very great impact on call handling performance. i have noticed this several times and i have written a solution which makes handle_statechange handling 61 times faster.
> 
> i have used the patch (rev. 293802 for 1.4) from russellb and jeffpeeler to also fix a deadlock problem and their part to move hints to an ao2_container.
> 
> after thinking about the best solution IMO iterating through the whole hint list/container to maybe find a hint with a matching device is very expensive way. So i have used a own container for devices which just have a link back to their hint. 
> This means there is a hashed cmp for devices which gives back a multiple object ao2_iterator for all hints, this device, which have a state change, is used in. 
> 
> these devices are not longer limited to any length so its possible to use a hint for 100 peers at once.
> 
> Tilghman Lesher noticed on the dev-list what would happen when a dynamic hint is used. I have tried this with one dynamic hint and subscribe up to 2500 different users on this, several times and it works just fine. If a subscribe for a dynamic hint comes in, a static extension with static appdata is generated which acts like a normal hint so there is no problem on this. 
> 
> 
> Diffs
> -----
> 
>   trunk/include/asterisk.h 294045 
>   trunk/main/asterisk.c 294045 
>   trunk/main/pbx.c 294045 
> 
> Diff: https://reviewboard.asterisk.org/r/1003/diff
> 
> 
> Testing
> -------
> 
> dialplan reload drops all devices out of the device container and recreate them by their callback list.
> core show hint / hints show all hints also dynamic and their watchers
> 
> 
> loadtests shows the following result:
> 
> orig version: 
> handle_statechange COUNTER: 10009 overalltime: 7041 ms 
> MAX: 365 CPS
> 
> patched version:
> handle_statechange COUNTER: 10009 overalltime: 115 ms
> MAX: 580 CPS
> 
> same setup, same config tested with sipp.
> 
> 
> Thanks,
> 
> schmidts
> 
>