[asterisk-dev] [Code Review] 3175: timing: Improve performance for most timing implementations

[Mark Michelson]

> On Feb. 4, 2014, 4:53 p.m., Corey Farrell wrote:
> > I like what you've done in general, I think the use of ao2 objects in timing implementations can be possibly stopped.
> > 
> > Most (if not all) users of struct ast_timer are already protected by a lock or are owned exclusively by a single thread.  If I'm right the locking within timing implementations are of no use, except maybe in res_timing_pthread.
> > 
> > As for refcounting, most of it has been removed since you no longer ao2_find the timer data.  Since timer lifetime is controlled by ast_timer_open / ast_timer_close, refcounting is not useful.
> 
> Joshua Colp wrote:
>     I thought so as well but after looking back at why locking exists in timerfd for example it was added because in certain cases it wasn't protected by a lock. I wanted to keep my changes as minimal as possible so as to not cause regressions in that area.

I'm in agreement with both of you with regards to the ao2 ref thing. Since keeping the diff minimal is a good idea, I think that removing refcounting from the kqueue, pthread, and timerfd implementations can wait.

However, I think that adding refcounting to the dahdi implementation is unnecessary and can be managed with a single malloc/free pairing.


- Mark


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


On Feb. 4, 2014, 3:16 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3175/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2014, 3:16 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The timing API is currently optimized for the case where timing implementations provide a file descriptor and any API invocations directly act on the file descriptor. In practice this turns out to not be the most common case. Timing implementations commonly store a structure with additional information and then have to use a container lookup on any API invocation to get to it.
> 
> The attached change removes this container lookup requirement by allowing implementations to store a pointer to their data directly. This removes the container lookup and the world is happier.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_timing_timerfd.c 407196 
>   /trunk/res/res_timing_pthread.c 407196 
>   /trunk/res/res_timing_kqueue.c 407196 
>   /trunk/res/res_timing_dahdi.c 407196 
>   /trunk/main/timing.c 407196 
>   /trunk/include/asterisk/timing.h 407196 
> 
> Diff: https://reviewboard.asterisk.org/r/3175/diff/
> 
> 
> Testing
> -------
> 
> Ran the timing test using the various timing implementations on Linux and confirmed they still work. I also placed some calls to a Playback and confirmed it works.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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