< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

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