>> + 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 _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev