[asterisk-dev] [Code Review] 2873: Stasis performance improvements

[rmudgett]

> On Sept. 24, 2013, 6:40 p.m., rmudgett wrote:
> > /branches/12/main/stasis_message_router.c, lines 217-219
> > <https://reviewboard.asterisk.org/r/2873/diff/3/?file=46134#file46134line217>
> >
> >     Is it possible for route.callback() to be removed by stasis_message_router_remove() by some other thread?  If so you should not change struct stasis_message_route from an ao2 object.
> 
> David Lee wrote:
>     The route variable is local to router_dispatch(), and it's filled in
>     by find_route() while the lock is held.
>     
>     So, no, route.callback() can't be changed by any other thread.

I was thinking more of the contents of the struct stasis_message_route becoming invalid after the route is removed.  The message_type member is a ref counted object that could become invalid when the route is removed.  The same with the data member becoming invalid after it is removed.  Calling the callback with the invalid data pointer would not be good.


- rmudgett


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


On Sept. 24, 2013, 4:29 a.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2873/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2013, 4:29 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch addresses several performance problems that were found in
> the initial performance testing of Asterisk 12.
> 
> The Stasis dispatch object was allocated as an AO2 object, even though
> it has a very confined lifecycle. This was replaced with a straight
> ast_malloc().
> 
> The Stasis message router was spending an inordinate amount of time
> searching hash tables. In this case, most of our routers had 6 or
> fewer routes in them to begin with. This was replaced with an array
> that's searched linearly for the route.
> 
> We more heavily rely on AO2 objects in Asterisk 12, and the memset()
> in ao2_ref() actually became noticeable on the profile. This was
> #ifdef'ed to only run when AO2_DEBUG was enabled.
> 
> After being misled by an erroneous comment in taskprocessor.c during
> profiling, the wrong comment was removed.
> 
> 
> Diffs
> -----
> 
>   /branches/12/include/asterisk/stasis_message_router.h 399650 
>   /branches/12/main/astobj2.c 399650 
>   /branches/12/main/stasis.c 399650 
>   /branches/12/main/stasis_message_router.c 399650 
>   /branches/12/main/taskprocessor.c 399650 
>   /branches/12/res/res_pjsip/include/res_pjsip_private.h 399650 
>   /branches/12/tests/test_stasis.c 399650 
> 
> Diff: https://reviewboard.asterisk.org/r/2873/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> Using the setup below, sipp was run against the 12 branch both with
> and without my changes.
> 
> ==
> [test-server]$ cat extensions.conf 
> 
> [performance]
> exten => service,1,Answer
> exten => service,n,Wait(1)
> exten => service,n,Hangup
> 
> [test-server]$ cat sip.conf 
> 
> [perf](!)
> type=peer
> qualify=no
> disallow=all
> allow=g722
> allow=ulaw
> insecure=invite,port
> context=performance
> 
> [test-client](perf)
> host=test-client
> 
> Simple sipp scenario:
> [test-client]$ sipp -sn uac -l 100 -r 50 tests-server
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130924/1989f766/attachment-0001.html>