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

[reviewboard at asterisk.org]

> On 2010-11-08 14:52:59, Tilghman Lesher wrote:
> > trunk/main/pbx.c, lines 1024-1029
> > <https://reviewboard.asterisk.org/r/1003/diff/2/?file=12836#file12836line1024>
> >
> >     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.

i have tested this with REF_DEBUG and its ok. Cause i use a refcounter value of 2 by default (ao2_alloc and ao2_link to container) not 1 like the hints which only have a refcounter value of 1 (ao2_link). If this would be a unclean way than i can remove the unref here and add it after the ao2_link so it will work with a refcounter of 1.


> On 2010-11-08 14:52:59, Tilghman Lesher wrote:
> > trunk/main/pbx.c, lines 986-994
> > <https://reviewboard.asterisk.org/r/1003/diff/2/?file=12836#file12836line986>
> >
> >     The comment here says CMP_STOP is not returned, yet it clearly is.

atleast this function should not be used at all cause a device from hints is not unique so there should allways be a iteration about the results.
i will remove this function.


> On 2010-11-08 14:52:59, Tilghman Lesher wrote:
> > trunk/main/pbx.c, lines 1069-1079
> > <https://reviewboard.asterisk.org/r/1003/diff/2/?file=12836#file12836line1069>
> >
> >     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.

lazy me. i will remove this.


- schmidts


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


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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.digium.com/pipermail/asterisk-dev/attachments/20101109/adbb1218/attachment-0001.htm