[reSIProcate] optimization of Data and Contents classes

[Scott Godin]

I've definitely used Preallocate in many of the apps I've worked on.  Unless
there is a really compelling reason to move it, I think you should avoid
that as well.

Scott

On Wed, Feb 9, 2011 at 12:55 PM, Kennard White
<kennard_white at logitech.com>wrote:

> Hi Bryon and Scott,
>
> Thanks for feedback. The maybe-NULL terminated aspect of Data is certainly
> tricky, and I suspect the greatest risk to the setBuf, etc., is that things
> may no longer be NULL-term, and (broken) code may assume that it is. I agree
> changes to Data::Empty and Data::npos are off the table.
>
> Also agree Data::Preallocate should remain a class for safety. Would moving
> Preallocate into DataHelper be acceptable? There are 40 usages within the
> tree to be fixed up (18 of which are in the Data class and its test
> programs). Do you expect there are many app usages of Preallocate?
>
> Thanks,
> Kennard
>
>
> On Tue, Feb 8, 2011 at 6:35 PM, Byron Campen <bcampen at estacado.net> wrote:
>
>>  Ok, looking at your patch now. Your modification to HeaderFieldValue
>> looks sane to me. Your modifications to SipMessage also look fine, and I
>> agree there was a bug in the way the encoding for the body was done in
>> SipMessage::encodeEmbedded() when the body was being stored as a
>> HeaderFieldValue. Good catch. I think the patches to Data are ok (the
>> maybe-I'm-null-terminated-maybe-I'm-not-so-mCapacity-doesn't-necessarily-mean-what-you-think-it-means
>> nature of Data makes me uncertain, but it looks like you've accounted for
>> this in every way I've checked). Can anyone else give this a look?
>>
>> As for your comment about LazyParser, Preallocate, and CopyEnumPadding;
>> LazyParser does not have 19 different c'tors, and isn't really a class that
>> the app is expected to use directly. Blowing your feet off by calling the
>> wrong LazyParser c'tor is far less probable than it is for Data.
>>
>> Best regards,
>> Byron Campen
>>
>> Hi Bryon,
>>
>> I've added your comments to template DataLocalSize into the header, thanks
>> for the detailed explanation.
>>
>> Re Preallocate, I see a lot of code in the style of
>> resip/stack/LazyParser, using HeaderFIeldValue::CopyEnumPadding as a marker.
>> This doesn't have any static cost nor gdb clutter. I agree that using a bool
>> as a marker doesn't seem safe.
>>
>> Anyways, seems like we're stuck with Preallocate, Empty and npos for
>> backward compat unless someone has a clever solution. I suspected that, but
>> thought I'd ask.
>>
>> Thanks,
>> Kennard
>>
>> On Mon, Feb 7, 2011 at 7:44 PM, Byron Campen <bcampen at estacado.net>wrote:
>>
>>>        Stuff inline:
>>>
>>> > Hi,
>>> > I came across this comment in the source code:
>>> >       // !bwc! This causes an additional copy; sure would be nice to
>>> have a way
>>> >       // to get a data to take on a buffer with Data::Share _after_
>>> construction
>>> >
>>> > One thing led to another, and I've added a number of methods to the
>>> Data class to enable different forms of sharing after construction. Please
>>> see attached patch. I hope to commit on Friday.
>>> >
>>>
>>>         I'll need to look at the patch tomorrow; a lot of stuff there.
>>>
>>> > These changes support writing code with fewer memory allocations and
>>> copies.
>>> >
>>> > I'd like to may a few other changes related to Data, but need some
>>> input:
>>> > 1. Remove the template<int S> struct DataLocalSize and just replace its
>>> usage with simple int. It seems like this is a holdover from when the whole
>>> class was templated. But I don't claim to fully understand templates -- is
>>> something more complex going on?
>>> >
>>>
>>>         This is there to help diagnose API/ABI mismatches. Say you build
>>> librutil. A single Data::init(DataLocalSize<RESIP_DATA_LOCAL_SIZE>) function
>>> is implemented in Data.cxx, with the default value.
>>> (Data::init(DataLocalSize<16>) ends up being defined,
>>> Data::init(DataLocalSize<15>) does not) If, later, another build using that
>>> librutil tries to tweak the local alloc size to 24, it will end up
>>> attempting to call Data::init(DataLocalSize<24>); this will result in a
>>> link-time error, that while opaque, is less opaque than the stack corruption
>>> that would result otherwise. If an implementor decides to build a "custom"
>>> librutil by fiddling with the local alloc size, they can by messing with the
>>> config.hxx include at the top of Data.hxx. They just have to make sure that
>>> every time they include Data.hxx (even in their own projects), they set the
>>> right value. This is a nightmare, I imagine, hence the link-time trick.
>>>
>>> > 2. I'd to replace class PreallocateType within an enum. Goal here is to
>>> get rid of static data Data::Preallocate, since this show up within gdb dump
>>> of every Data object, and make classes like SipMessage nearly unreadable. Is
>>> there some reason the pre-allocating constructor was declared with a
>>> structure disambiguating argument rather than an enum? Is it because of
>>> potential automatic conversion between ints and enum?
>>> >
>>>
>>>         PreallocateType is definitely a class to avoid implicit type
>>> conversions, especially considering that the only function it is used in
>>> uses it as a dummy disambiguation parameter. In fact, I'm betting that
>>> someone got bit by the bool version of the same function, and added this to
>>> keep themselves from being bit again! To be honest, however, I am not sure
>>> why this c'tor even exists. Data::reserve() already exists, and the default
>>> Data c'tors don't allocate any buffers in the first place. I suppose there
>>> might be a tiny performance gain associated with member initialization when
>>> everything is done in the c'tor, but it is not going to be much.
>>>
>>> > 3. Related to cleaning up static data members, I'd like to move
>>> Data::Empty to DataHelper::Empty. This would impact a lot of code, and
>>> unfortunately I'm guessing a lot of application code. Is this too big of a
>>> change to consider?
>>>
>>>         Yeah, the user-base might not like you if you do that. I do agree
>>> that it impacts readability of gdb dumps, but I'm not coming up with any
>>> clever ideas for removing it without upsetting existing builds without using
>>> really nasty macros (#define Data::Empty Data::getEmpty(); where
>>> Data::getEmpty() is a static function that returns a ref to an empty Data
>>> declared outside of class scope).
>>>
>>> >
>>> > 4. And again related to cleaning up static data members, I'd like to
>>> move Data::npos to to DataHelper::npos. I'm guessing this has limited
>>> impact, but still has potential to break application code. Thoughts?
>>>
>>>         Same as point 3.
>>>
>>> Best regards,
>>> Byron Campen
>>>
>>>
>>
>>
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20110209/9e3abc07/attachment.htm>