[squid-dev] [PATCH] Extend SBufContainerJoin to have prefix and suffix arguments

Kinkie 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,
>   and/or
> * 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.

v4 attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbufcontainerjoin-v4.patch
Type: application/octet-stream
Size: 6642 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161110/76f6b98c/attachment.obj>

More information about the squid-dev mailing list