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

[Alex Rousskov]

On 11/04/2016 01:12 AM, Kinkie wrote:
> On Thu, Nov 3, 2016 at 10:55 PM, Alex Rousskov
> <rousskov at measurement-factory.com> wrote:
>> On 11/03/2016 03:19 PM, Kinkie wrote:
>>> On Tue, Nov 1, 2016 at 8:47 PM, Alex Rousskov wrote:
>>>> On 11/01/2016 02:02 PM, Kinkie wrote:
>>>>>   the attached patch extends SBufContainerJoin to have prefix and
>>>>> suffix arguments.
>>
>>>> I recommend reworking this by adding a dest parameter:
>>>> SBuf &ContainerToSBuf(SBuf &dest, ...);

> attached v3 does this.

>  SBuf
> +JoinContainerIntoSBuf(SBuf &dest, ...

Please return dest, not a copy of it. There is no reason to create and
destroy a new SBuf object here IMO. SBuf creation and destruction are
not as expensive as underlying storage copying, but they are still a
measurable expense without justification AFAICT.

There are certainly many cases where using references is a bad idea but
returning a fed-by-reference "dest" is not one of them AFAICT. In fact,
it is nearly the norm in "chainable" calls such as those that append
something.


> +/// variant of JoinContainerIntoSBuf not needing a SBuf lvalue

I recommend a more informative description than retelling of the
function declaration code. For example:

/// convenience JoinContainerIntoSBuf() wrapper with an extra memory
allocation

> +    dest.reserveSpace(sz + prefix.length() + suffix.length());

I would reorder the operands to make the code more self-documenting,
especially when using an anonymous variable name like "sz":

  dest.reserveSpace(prefix.length() + sz + suffix.length());


> I am not generally worried as it seems you are about copying SBufs as
> long as the underlying storage is not reallocated needlessly. 

Unfortunately, all three patch versions reallocate storage needlessly
(for various reasons). You can study the attached debugging from the
attached patch to see what is actually going on in various
JoinContainerIntoSBuf implementations. It may surprise and inspire you
to make the code better. It did surprise and inspire me (but I wish I
was not doing this legwork!).


Thank you,

Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbuf-alloc.log
Type: text/x-log
Size: 8233 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161104/3b8dcbc9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbuf-alloc.patch
Type: text/x-diff
Size: 3301 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161104/3b8dcbc9/attachment.patch>