[asterisk-dev] [Code Review] 2883: Performance: Optimize how Stasis forwards are dispatched

[David Lee]

> On Sept. 26, 2013, 12:26 p.m., Mark Michelson wrote:
> > /team/dlee/taskprocessor-optimization/include/asterisk/vector.h, lines 123-127
> > <https://reviewboard.asterisk.org/r/2883/diff/1/?file=46385#file46385line123>
> >
> >     It's hard for me to imagine a situation where this would be useful or safe to use, since the index where a particular element is stored can potentially change.
> >     
> >     In addition, couldn't this leak memory if the caller of ast_vector_remove_unordered() does not have a reference to the element being removed?
> >     
> >     I like the idea of constant time removal from a vector, but in order to do it properly, I think you'd need to require that any item that can be placed in a vector be instrumented in some way to allow for the vector to track the element's index, like how AST_LIST_ENTRY() works for linked lists. That way, ast_vector_remove_elem_unordered() could work in constant time by using the stored index on the element (and also updating the stored index on the element that is moved).
> >     
> >     Of course, this introduces the idea that vector operations can actually modify the elements in the vector, which may be something you were avoiding.

Removal from the middle of a vector always requires the indexes of
elements to change. Otherwise, your vector will have a gap in the
middle.

Resource management is the responsibility of the caller, but I can see
where that can be difficult when removing by index. The macro can
return the removed element, so the caller can more easily unref/free
it.

As far as putting information in the element itself, the problem is
that an element may be in more than one vector, which makes storing
information in the element impossible.


- David


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


On Sept. 26, 2013, 8:38 a.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2883/
> -----------------------------------------------------------
> 
> (Updated Sept. 26, 2013, 8:38 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch optimizes how forwards are dispatched in Stasis.
> 
> Originally, forwards were dispatched as subscriptions that are invoked
> on the publishing thread. This did not account for the vast number of
> forwards we would end up having in the system, and the amount of work it
> would take to walk though the forward subscriptions.
> 
> This patch modifies Stasis so that rather than walking the tree of
> forwards on every dispatch, when forwards and subscriptions are changed,
> the subscriber list for every topic in the tree is changed.
> 
> This has a couple of benefits. First, this reduces the workload of
> dispatching messages. It also reduces contention when dispatching to
> different topics that happen to forward to the same aggregation topic
> (as happens with all of the channel, bridge and endpoint topics).
> 
> Since forwards are no longer subscriptions, the bulk of this patch is
> simply changing stasis_subscription objects to stasis_forward objects
> (which, admittedly, I should have done in the first place.)
> 
> Since this required me to yet again put in a growing array, I finally
> abstracted that out into a set of ast_vector macros in
> asterisk/vector.h.
> 
> 
> Diffs
> -----
> 
>   /team/dlee/taskprocessor-optimization/apps/app_queue.c 399869 
>   /team/dlee/taskprocessor-optimization/include/asterisk/stasis.h 399869 
>   /team/dlee/taskprocessor-optimization/include/asterisk/vector.h PRE-CREATION 
>   /team/dlee/taskprocessor-optimization/main/cdr.c 399869 
>   /team/dlee/taskprocessor-optimization/main/cel.c 399869 
>   /team/dlee/taskprocessor-optimization/main/channel_internal_api.c 399869 
>   /team/dlee/taskprocessor-optimization/main/manager.c 399869 
>   /team/dlee/taskprocessor-optimization/main/manager_bridges.c 399869 
>   /team/dlee/taskprocessor-optimization/main/manager_channels.c 399869 
>   /team/dlee/taskprocessor-optimization/main/manager_mwi.c 399869 
>   /team/dlee/taskprocessor-optimization/main/manager_system.c 399869 
>   /team/dlee/taskprocessor-optimization/main/stasis.c 399869 
>   /team/dlee/taskprocessor-optimization/main/stasis_cache_pattern.c 399869 
>   /team/dlee/taskprocessor-optimization/res/stasis/app.c 399869 
>   /team/dlee/taskprocessor-optimization/tests/test_stasis.c 399869 
>   /team/dlee/taskprocessor-optimization/tests/test_stasis_endpoints.c 399869 
> 
> Diff: https://reviewboard.asterisk.org/r/2883/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> Performance testing to verify the speed up.
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130927/ce08f476/attachment.html>