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