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

[Kinkie]

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

> * either your patch (mimicking my JoinContainerIntoSBuf4() which does
> not result in extra allocation) reserveSpace()

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

In other words, the test data is dirtied as the tests progress.
Follow with me:
2016/11/04 11:47:21.841| 24,8| SBuf.cc(42) SBuf: SBuf2698 created
2016/11/04 11:47:21.841| 24,8| SBuf.cc(908) cow: SBuf2698 new size:1024
2016/11/04 11:47:21.841| 24,8| SBuf.cc(878) reAlloc: SBuf2698 new size: 1024

But then:

2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(47)
JoinContainerIntoSBuf2: started
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(48)
JoinContainerIntoSBuf2: did not create an empty buf
2016/11/04 11:47:21.841| 24,8| SBuf.cc(908) cow: SBuf2698 new size:124
2016/11/04 11:47:21.841| 24,8| SBuf.cc(878) reAlloc: SBuf2698 new size: 124

This is correct behavior: the code advises that it only needs that
much storage and the SBuf shrinks. It's passed by reference, so from
now on 'buffer' has 124 bytes of storage instead of the pre-allocated
1024.

As a demonstration, swapping order of tests 3 and 4 results in:
[...]
2016/11/06 07:05:47.860 kid1| 1,2| main.cc(457) JoinContainerIntoSBuf4: started
2016/11/06 07:05:47.860 kid1| 24,8| SBuf.cc(130) reserve: SBuf3657
was: 0+81+47=128
2016/11/06 07:05:47.860 kid1| 24,8| SBuf.cc(912) cow: SBuf3657 new size:181
2016/11/06 07:05:47.860 kid1| 24,8| SBuf.cc(882) reAlloc: SBuf3657 new size: 181
2016/11/06 07:05:47.860 kid1| 24,8| MemBlob.cc(101) memAlloc: blob2420
memAlloc: requested=181, received=512
2016/11/06 07:05:47.860 kid1| 24,7| SBuf.cc(891) reAlloc: SBuf3657 new
store capacity: 512
2016/11/06 07:05:47.860 kid1| 24,7| SBuf.cc(146) reserve: SBuf3657
now: 0+81+431=512
2016/11/06 07:05:47.860 kid1| 1,2| main.cc(462)
JoinContainerIntoSBuf4: reserved space
2016/11/06 07:05:47.860 kid1| 24,7| SBuf.cc(154) rawSpace: reserving 7
for SBuf3657
2016/11/06 07:05:47.860 kid1| 24,7| SBuf.cc(161) rawSpace: SBuf3657 not growing


I don't think however that we should change reserveSpace() behavior
not to ever shrink; there may be use cases where it's better not to do
it in fact. At the same time, it's not a matter of API: the extended
reserve() has the same behavior as the "problem" is elsewhere.

>
> * or reserveSpace() (after making sure that all callers are going to be
> OK with that change).
>
> The latter would be better if it is possible. If it is not possible,
> then we may need to change reserveSpace() documentation so that folks do
> not use that method like you did.

What we could do is:
- define a reserveMinSpace() which does not shrink storage
- reimplement both reserveSpace() and reserveMinSpace() as convenience
wrappers around reserve() which is undoubtably more powerful
- while we're at it, give SBufReservationRequirements a convenience constructor

What do you think?
Thanks.

-- 
    Francesco