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

[Alex Rousskov]

On 11/06/2016 01:15 AM, Kinkie wrote:
>>> +    dest.reserveSpace(prefix.length() + totalContainerSize + suffix.length());
>>
>> Please note that v4 still allocates memory according to my last
>> experiment. See JoinContainerIntoSBuf3() which mimics your patch v4. You
>> may claim that the unnecessary allocation is not the fault of this
>> patch, and you could be right, but I was hoping this observation will
>> inspire you to change
> 
> I don't follow. The space requirement in JoinContainerIntoSBuf is not
> advisory. The accumulate at the beginning of the function is meant to
> calculate exactly how much space is needed, to copy (if needed) only
> once and early.

What you say is true except for the "if needed" part. In some cases,
JoinContainerIntoSBuf() allocates and copies without needed. Those extra
allocations are present in v1, v2, v3, and v4 versions of the patch, but
the reasons behind those unnecessary allocations varied.


> The traces you provided do not reallocate there because the underlying
> MemBlob has grown enough in previous calls to have enough free space
> to cover the projected needs (it was resized last at
> JoinContainerIntoSBuf3).

No, "has grown enough" is not the reason why JoinContainerIntoSBuf4()
does not reallocate needlessly.


> In other words, the test data is dirtied as the tests progress.

My tests were indeed dirty (sorry) and could have easily led to wrong
conclusions, but they did not. The cleaned up version (attached) shows
exactly the same problem, this time without any effect of earlier tests
on later tests.


> This is correct behavior: the code advises that it only needs that
> much storage and the SBuf shrinks.

Violating explicit STL reserve() guarantees is rarely a good idea but,
for now, I will not argue about what reserveSpace() should do. I will
focus on JoinContainerIntoSBuf() instead.

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.


> As a demonstration, swapping order of tests 3 and 4 results in:

[Snipped wrong conclusions based on confusing dirty tests.
 See cleaned up order-independent tests instead.]


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



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


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


HTH,

Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbuf-alloc-t2.patch
Type: text/x-diff
Size: 4098 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161106/7b6067aa/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbuf-alloc-t2.log
Type: text/x-log
Size: 11932 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161106/7b6067aa/attachment-0001.bin>