[asterisk-dev] [Code Review] 2879: Cache format strings to prevent excessive container traversals

[David Lee]

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


Looks good. Just a minor suggestion.


/team/group/performance/main/format_cap.c
<https://reviewboard.asterisk.org/r/2879/#comment18999>

    Maybe this would be a micro-optimization, and not make a difference, but...
    
    Instead of generating the string every time the format_cap changes, you can add a 'cache_valid' flag that gets unset.
    
    ast_getformatname_multiple() can test the flag, and update the cache if needed.


- David Lee


On Sept. 24, 2013, 5:42 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2879/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2013, 5:42 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee and Joshua Colp.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> While profiling Asterisk 12, it became apparent that ao2 container traversals account for a high percentage of CPU time. Drilling down a bit reveals that the heaviest user is the function ast_getformatname_multiple() in main/format_cap.c. Every time a channel snapshot is created, the native formats on the channel are traversed in order to create the string. Applying some smart caching could save some unnecessary traversals since native formats on a channel do not change often.
> 
> Due to the nature of ast_format_cap(), the best way to improve the situation was to modify main/format_cap.c to optionally cache the string representation of formats any time the format_cap changes. Rather than creating more permutations of allocations for ast_format_cap(), I consolidated into a single allocation function that takes a set of flags to determine behavior. If AST_FORMAT_CAP_FLAG_CACHE_STRINGS is passed in when allocating the ast_format_cap, then any updates will result in caching the string representation of the formats. This way, in the many channel snapshot creations that occur, the strings are simply copied from the ast_format_cap into the snapshot rather than having to re-build the strings each time.
> 
> As a secondary change, I also decreased the number of buckets in the format container from 283 to 11 since it is quite rare for a channel to have many formats on it at all.
> 
> Note to reviewers: the main bulk of what to look at here is in main/format_cap.c. Since I changed ast_format_cap_alloc(), I had to make changes all over the place. I used a sed script to make the changes, so there should be little risk of having made mistakes. The only allocation that has actually been changed is the allocation of channel native formats in main/channel.c
> 
> 
> Diffs
> -----
> 
>   /team/group/performance/addons/chan_mobile.c 399697 
>   /team/group/performance/addons/chan_ooh323.c 399697 
>   /team/group/performance/apps/app_confbridge.c 399697 
>   /team/group/performance/apps/app_meetme.c 399697 
>   /team/group/performance/apps/app_originate.c 399697 
>   /team/group/performance/bridges/bridge_holding.c 399697 
>   /team/group/performance/bridges/bridge_native_rtp.c 399697 
>   /team/group/performance/bridges/bridge_simple.c 399697 
>   /team/group/performance/bridges/bridge_softmix.c 399697 
>   /team/group/performance/channels/chan_alsa.c 399697 
>   /team/group/performance/channels/chan_bridge_media.c 399697 
>   /team/group/performance/channels/chan_console.c 399697 
>   /team/group/performance/channels/chan_dahdi.c 399697 
>   /team/group/performance/channels/chan_gtalk.c 399697 
>   /team/group/performance/channels/chan_h323.c 399697 
>   /team/group/performance/channels/chan_iax2.c 399697 
>   /team/group/performance/channels/chan_jingle.c 399697 
>   /team/group/performance/channels/chan_mgcp.c 399697 
>   /team/group/performance/channels/chan_misdn.c 399697 
>   /team/group/performance/channels/chan_motif.c 399697 
>   /team/group/performance/channels/chan_multicast_rtp.c 399697 
>   /team/group/performance/channels/chan_nbs.c 399697 
>   /team/group/performance/channels/chan_oss.c 399697 
>   /team/group/performance/channels/chan_phone.c 399697 
>   /team/group/performance/channels/chan_pjsip.c 399697 
>   /team/group/performance/channels/chan_sip.c 399697 
>   /team/group/performance/channels/chan_skinny.c 399697 
>   /team/group/performance/channels/chan_unistim.c 399697 
>   /team/group/performance/channels/dahdi/bridge_native_dahdi.c 399697 
>   /team/group/performance/include/asterisk/format_cap.h 399697 
>   /team/group/performance/main/bridge_basic.c 399697 
>   /team/group/performance/main/ccss.c 399697 
>   /team/group/performance/main/channel.c 399697 
>   /team/group/performance/main/core_local.c 399697 
>   /team/group/performance/main/dial.c 399697 
>   /team/group/performance/main/file.c 399697 
>   /team/group/performance/main/format_cap.c 399697 
>   /team/group/performance/main/manager.c 399697 
>   /team/group/performance/main/media_index.c 399697 
>   /team/group/performance/main/rtp_engine.c 399697 
>   /team/group/performance/pbx/pbx_spool.c 399697 
>   /team/group/performance/res/ari/resource_bridges.c 399697 
>   /team/group/performance/res/parking/parking_applications.c 399697 
>   /team/group/performance/res/res_agi.c 399697 
>   /team/group/performance/res/res_clioriginate.c 399697 
>   /team/group/performance/res/res_pjsip/pjsip_configuration.c 399697 
>   /team/group/performance/res/res_pjsip_sdp_rtp.c 399697 
>   /team/group/performance/res/res_pjsip_session.c 399697 
>   /team/group/performance/res/res_stasis.c 399697 
>   /team/group/performance/tests/test_config.c 399697 
>   /team/group/performance/tests/test_format_api.c 399697 
> 
> Diff: https://reviewboard.asterisk.org/r/2879/diff/
> 
> 
> Testing
> -------
> 
> Ran a SIPp test that sent 25 cps at Asterisk 12 (using chan_sip) with a maximum of 100 calls up at any given time. Dialplan for these calls simply answered, waited a second, and hung up. With these changes, no problems were encountered. top showed CPU percentage decrease by 1 to 2 percent on a test like this. When the cps and max calls were increased, the CPU savings increased linearly (i.e. 50 cps saved 2-4 % CPU).
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

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