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

[Mark Michelson]

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



/team/dlee/taskprocessor-optimization/include/asterisk/vector.h
<https://reviewboard.asterisk.org/r/2883/#comment19005>

    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.



/team/dlee/taskprocessor-optimization/include/asterisk/vector.h
<https://reviewboard.asterisk.org/r/2883/#comment19006>

    I may be overthinking, but the idea of providing an optional comparator for this macro could be useful for cases where a simple == will not due for comparing vector elements.



/team/dlee/taskprocessor-optimization/main/stasis.c
<https://reviewboard.asterisk.org/r/2883/#comment19007>

    To prevent future potential deadlocks, you should document here that if you wish to lock both the from and to topic, then you must lock the to topic first, then the from topic.


- Mark Michelson


On Sept. 26, 2013, 1:38 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2883/
> -----------------------------------------------------------
> 
> (Updated Sept. 26, 2013, 1:38 p.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/20130926/73987a31/attachment-0001.html>