[squid-dev] [PATCH] Increase request buffer size to 64kb

[Alex Rousskov]

On 03/31/2016 11:22 PM, Nathan Hoad wrote:
> I've attached two patches - they're functionally identical, one uses
> SBuf and the other using MemBuf.

I am only looking at SBuf patch because MemBuf is deprecated. I have not
read your analysis email yet, but will do so soon. Meanwhile, I wanted
to give you something to work on...


> Make the size of the buffer on Http::Stream configurable.

s/buffer on Http::Stream/ClientStream buffer/


Please do mention the other two equally (or more!) important changes:

* Enlarge that buffer [default] size (from 4KB to 64KB) to ...

* Enlarge read_ahead_gap (from 4KB to 64KB) to ...

The code changes will show what you did, but will not explain *why* you
are changing these defaults. Do not let your research be forgotten in
the mailing list archives!


> +StoreIOBuffer
> +Http::Stream::getStoreIOBuffer()
> +{
> +    StoreIOBuffer tempBuffer;
> +    tempBuffer.data = this->requestBuffer.rawSpace(Config.httpStreamBufferSize);
> +    tempBuffer.length = this->requestBuffer.spaceSize();
> +    assert(tempBuffer.length);
> +    return tempBuffer;
> +}

getStoreIOBuffer() name tells us that the method returns a
StoreIOBuffer, which we already know based on the method return type. I
suggest calling this method clientStreamBuffer() to tell the reader what
kind of StoreIOBuffer the method returns.


Adding a method that grows some buffer without bounds on every call is a
bad idea in general. I suspect this problem will go away when you
convert to MemBlob (because it will be allocated once).


The new assert() looks out of place and/or misleading: rawSpace()
guarantees spaceSize() to be at least Config.httpStreamBufferSize. Thus,
the assert() essentially checks that Config.httpStreamBufferSize is
positive. getStoreIOBuffer is the wrong place to check configuration
[and assert if it incorrect]. Moreover, since getStoreIOBuffer() does
not try to write something into that buffer, it does not actually care
whether tempBuffer.length is zero and should not assert if it is.

Please find a better place to check that Config.httpStreamBufferSize is
positive. There should be some configuration validation code in Squid
already.


Please no explicit "this" inside regular class methods. There are some
rare cases where using explicit "this" is appropriate or even required,
but this is not one of them.



> +    requestBuffer.reserveCapacity(Config.httpStreamBufferSize);

This is premature RAM spending. Your code always requests at least
Config.httpStreamBufferSize space bytes later so there is no need to
pre-allocate the same amount here. However, you may have to do that once
you migrate away from SBuf to MemBlob (unless you allocate MemBlob when
asked for the stream buffer).


> -    StoreIOBuffer tempBuffer;
> -    tempBuffer.data = context->reqbuf;
> -    tempBuffer.length = HTTP_REQBUF_SZ;
> +    StoreIOBuffer tempBuffer = context->getStoreIOBuffer();
>      clientStreamInit(&http->client_stream, clientGetMoreData, clientReplyDetach,
>                       clientReplyStatus, new clientReplyContext(http), clientSocketRecipient,
>                       clientSocketDetach, context, tempBuffer);

Please remove the used-once tempBuffer variable to emphasize that the
buffer is only used for ClientStreams. Just call getStoreIOBuffer() when
calling clientStreamInit(). Same in other similar contexts you modify.


> +NAME: http_stream_buffer_size

The option applies to the FTP ClientStream buffer as well, so this name
does not seem to work at all. Please also avoid the overloaded "stream"
word; there are too many "streams" floating around and it gets confusing.

I cannot suggest a good alternative, but something like
"internal_client_response_buffer_size" would be better IMO. It would be
good to emphasize that this is something low-level that may change or
disappear. Technically, this buffer should not even exist (at least not
in the current form)...


> +	The buffer size that the cache will use when streaming response objects to
> +	clients.

I recommend avoiding overloaded words like "cache" and "streaming". How
about this (*if* it is true; adjust as needed!):

"Specifies the size of one of the internal buffers that Squid uses for
delivering responses to clients. Network writes are done from this
buffer. This buffer is used for both hits and misses. Reducing the
buffer size does XXX. Enlarging the buffer improves YYY.

Squid does _not_ slow down response delivery to the client in order to
fill the buffer. This is one of several internal buffers a response may
go through. It may eventually be optimized away."


Fill XXX and YYY as needed to illustrate the trade-off the new parameter
controls -- there is little point in documenting this configuration
parameter if we do not tell the admin what the parameter affects. This
is very important IMO.

Again, adjust as needed.


> The patches are also abusing
> MemBuf/SBuf - they're being used purely to allow configuration of the
> buffer size, they're not being used as they normally are, or growing
> dynamically. That would involve quite a few more changes, I think.

Yeah. Dynamic resizing may require rewriting ClientStreams. I would not
go there unless you have a lot of time to devote to that noble task.


Nevertheless, SBuf is the wrong class for this task, as you have noticed
yourself. If possible, please use MemBlob instead. You can find examples
of that class being used for similar tasks in TcpLogger and Rock::IoState.


N.B. Please use 20-30 lines of context when posting future patches --
more context makes them easier to review correctly. The default is 3-4
lines. Also, if you can configure your diff program to add function
names to patch hunks, please do so (--show-c-function for Linux diffs).


Thank you,

Alex.