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

[Kinkie]

> 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.

Yes.

> 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.

Yes.


>> - 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.

Yes.

> 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.

-- 
    Francesco
-------------- 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>