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