Re: [reSIProcate] optimization of Data and Contents classes
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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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