[reSIProcate] optimization of Data and Contents classes

[Kennard White]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20110207/788fbd93/attachment.htm>