[asterisk-dev] [Code Review] 2861: CDRs: Fix a performance problem caused by unneeded container traversals

[Mark Michelson]

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



./branches/12/main/cdr.c
<https://reviewboard.asterisk.org/r/2861/#comment18899>

    You changed the hash function to match the template, but not the cmp function.



./branches/12/main/cdr.c
<https://reviewboard.asterisk.org/r/2861/#comment18900>

    Just a suggestion, but if you change to a for loop here, then you can remove the individual ao2_ref(channel_id, -1) calls from within the loop and have it be part of the for loop.
    
    for (it_cdrs = ao2_iterator_init(bridge->channels, 0);
            !success && (channel_id = ao2_iterator_next(&it_cdrs));
            ao2_ref(channel_id, -1)) {
        ...
    }
    
    Then the only time you'd have to call ao2_ref(channel_id, -1) explicitly in the loop is if you have a break.
    
    This same suggestion applies to all ao2_iterator while loops in this changeset.


- Mark Michelson


On Sept. 17, 2013, 2:44 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2861/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2013, 2:44 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22488
>     https://issues.asterisk.org/jira/browse/ASTERISK-22488
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Yes, this is a CDR patch: but it actually simplifies things! (For as much as they can be simplified)
> 
> This patch fixes a potential problem pointed out by Richard while he worked on the masquerade super test involving a lot of traversals of a container that has an entry for every channel in the system.
> 
> Some background:
> 
> All channels have a CDR. Sometimes, that CDR is doomed to die (as in the case of a dialed channel), but they have one anyways - usually in case they get ejected out of the bridge. This means that our main container that tracks CDRs will have an entry for every channel. In addition, we process both channel snapshot updates, as well bridge updates. Bridge updates have a bridge snapshot, which contains a container of the unique IDs of the channels in the bridge.
> 
> This patch:
> 
> The portion of the CDR logic that this patch deals with is how we make pairings when a channel enters a mixing bridge. In general, when a channel enters such a bridge, we need to do two things:
>  (1) Figure out if anyone in the bridge can be this channel's Party B.
>  (2) Make pairings with every other channel in the bridge that is not already our Party B.
> 
> This is a two step process. In the first step, we look through everyone in the bridge and see if they can be our Party B (single_state_process_bridge_enter). If they can - yay! We mark our CDR as having gotten a Party B. If not, we keep searching. If we don't find one, we wait until someone joins who can be our Party B.
> 
> Step 2 is where we changed the logic (handle_bridge_pairings and bridge_candidate_process). Previously, we would first find candidates - those channels in the bridge with us - from the active_cdrs_by_channel container. Because a channel could be a candidate if it was Party B to an item in the container, the code implemented multiple ao2_container callbacks to get all the candidates. We also had to store them in another container with some other meta information. This got rather complex and costly, particularly if you have 300 Local channels (600 channels!) going at once.
> 
> Luckily, none of it is needed: when a channel enters a bridge (which is when we're figuring all this stuff out), the bridge snapshot tells us the unique IDs of everyone already in the bridge. All we need to do is:
>  For all channels in the bridge:
>    If the channel is us or our Party B that we got in step 1, skip it
>    Compare us and the candidate to figure out who is Party A (based on some specific rules)
>    If we are Party A:
>       Make a new CDR for us, append it to our chain, and set the candidate as Party B
>    If they are Party A:
>       If they don't have a Party B:
>         Make a new CDR for them, append us to their chain, and us as Party B
>       Otherwise:
>         Copy us over as Party B on their existing CDR.
> 
> This patch does that.
> 
> Because we now use channel unique IDs to find the candidates during bridging, active_cdrs_by_channel now looks up things using uniqueid instead of channel name. This makes the more complex code simpler; it does, however, have the drawback that dialplan applications and functions will be slightly slower as they have to iterate through the container looking for the CDR by name. That's a small price to pay however as the bridging code will be called a lot more often.
> 
> 
> Diffs
> -----
> 
>   ./branches/12/main/cdr.c 399220 
> 
> Diff: https://reviewboard.asterisk.org/r/2861/diff/
> 
> 
> Testing
> -------
> 
> All unit tests pass. Note that these test multiple channels in a bridge and - once the code was written correctly - they all passed.
> 
> Expected test suite tests still pass.
> 
> Ran under valgrind; no memory leaks in CDRs.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130917/646496cd/attachment-0001.htm>