[asterisk-dev] [Code Review] Optimizations to the stringfields API

[Kevin Fleming]

> On 2009-02-17 18:11:05, Mark Michelson wrote:
> > /trunk/include/asterisk/stringfields.h, line 273
> > <http://reviewboard.digium.com/r/165/diff/1/?file=2796#file2796line273>
> >
> >     This comparison is not necessary since __ast_string_field_ptr_grow will return 0 if this is the case. See the grow <= 0 case in __ast_string_field_ptr_grow.

That's true, although this check is faster than making a function call.


> On 2009-02-17 18:11:05, Mark Michelson wrote:
> > /trunk/include/asterisk/stringfields.h, line 276
> > <http://reviewboard.digium.com/r/165/diff/1/?file=2796#file2796line276>
> >
> >     Correct me if I'm wrong, but it seems to me this if statement will never evaluate true. I understand the idea behind it, but I don't think this if statement captures the logic correctly. Perhaps you meant to do
> >     
> >     if (*__p__ != *(ptr)) {
> >     
> >     ?

You are correct; fixed.


> On 2009-02-17 18:11:05, Mark Michelson wrote:
> > /trunk/main/utils.c, lines 1483-1494
> > <http://reviewboard.digium.com/r/165/diff/1/?file=2797#file2797line1483>
> >
> >     I would feel warmer and fuzzier inside if these "48's" were replaced with some named constant.

Done.


> On 2009-02-17 18:11:05, Mark Michelson wrote:
> > /trunk/main/utils.c, line 1576
> > <http://reviewboard.digium.com/r/165/diff/1/?file=2797#file2797line1576>
> >
> >     I think all these hardcoded 2's should be replaced with sizeof(unsigned short). I've never seen a system where an unsigned short is not 2 bytes, but still, it can't hurt to be absolutely safe.

Done, along with adding a macro to encapsulate the pointer math involved in finding the allocation size field.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/165/#review455
-----------------------------------------------------------


On 2009-02-17 17:11:08, Kevin Fleming wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/165/
> -----------------------------------------------------------
> 
> (Updated 2009-02-17 17:11:08)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch provides a number of optimizations to the stringfields API, focused around saving (not wasting) memory whenever possible. Thanks to Mark Michelson for inspiring this work and coming up with the first two optimizations that are represented here:
> 
> Changes:
> 
> - Cleanup of some code, fix incorrect doxygen comments
> 
> - When a field is emptied or replaced with a new allocation, decrease the amount of 'active' space in the pool it was held in; if that pool reaches zero active space, and is not the current pool, then free it as it is no longer in use
> 
> - When allocating a pool, try to allocate a size that will fit in a 'standard' malloc() allocation without wasting space
> 
> - When allocating space for a field, store the amount of space in the two bytes immediately preceding the field; this eliminates the need to call strlen() on the field when overwriting it, and more importantly it 'remembers' the amount of space the field has available, even if a shorter string has been stored in it since it was allocated
> 
> - Don't automatically double the size of each successive pool allocated; it's wasteful
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/stringfields.h 176771 
>   /trunk/main/utils.c 176771 
> 
> Diff: http://reviewboard.digium.com/r/165/diff
> 
> 
> Testing
> -------
> 
> Compile testing only, so far.
> 
> 
> Thanks,
> 
> Kevin
> 
>