[squid-dev] [PATCH] Extend SBufContainerJoin to have prefix and suffix arguments
gkinkie at gmail.com
Thu Nov 10 20:28:15 UTC 2016
> Tests and code analysis show that JoinContainerIntoSBuf() allocates when
> no allocation is needed. I have also shown that it is possible to
> implement JoinContainerIntoSBuf() so that there is no extra allocation.
> AFAICT, going forward, you choices are:
> * state that the extra allocation is not your fault, leaving it as is,
> * fix JoinContainerIntoSBuf() to avoid the extra allocation.
> When you added the "dest" parameter, you have started moving in the
> latter direction.
I'm completing that by using the reseve() based approach
>> At the same time, it's not a matter of API: the extended
>> reserve() has the same behavior as the "problem" is elsewhere.
> Whether SBuf::reserve() has the same behavior as reserveSpace() depends
> on reserve() configuration parameters.
> JoinContainerIntoSBuf() should configure reserve() in such a way that
> reserve() does _not_ have the same behavior as reserveSpace(). I showed
> how to do that in JoinContainerIntoSBuf4() in my earlier patch. The
> patch attached now has an even simpler JoinContainerIntoSBuf5()
> implementation because I fixed reserve() to not require the .ideal
> configuration parameter to be set (trunk r14917).
Aha! I was confused as I had branched off earlier. Merging.
>> What we could do is:
>> - define a reserveMinSpace() which does not shrink storage
> Having two methods called reserveMinSpace() and reserveSpace(), one of
> which does not shrink and the other one does, makes no sense to me
> because the method names do not reflect that difference at all and
> because reserve() is always primarily about "minimum" space anyway.
> More importantly, reserveSpace() guarantees single ownership. IMO,
> shrinking is essentially a side effect of that guarantee.
> I suspect we do not need a "provide single-ownership space, shrinking
> the existing single-owned MemBlob if there is one" method, but if you
> have some existing use cases for it, please share them. If there are no
> such cases, reserveSpace()/reserveCapacity() should be optimized to
> avoid unnecessary re-allocation when their MemBlob is already not
> shared, but that is a different story, unrelated to JoinContainerIntoSBuf!
I agree. And that's why I think they could be simply convenience
methods for reserve().
>> - reimplement [...] reserveSpace() as convenience wrapper around reserve()
> Sure, but that is outside this patch scope.
>> - while we're at it, give SBufReservationRequirements a convenience constructor
> With minSpace as the only parameter? Sure. Please do not forget
> "explicit". Again, that seems to be outside your patch scope.
> I do not recommend providing more than one constructor parameter though
> because constructors with multiple same- or similar-type parameters are
> a recipe for mismatching parameters disasters.
Let's optimize the common case, we'll see if there is a need for more later on.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 6642 bytes
Desc: not available
More information about the squid-dev