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

[Nathan Hoad]

Hello,

One large reply again. Attached is a patch with all the earlier
changes recommended by Alex. Most notably this removes the change for
read_ahead_gap, leaving the default at 16kb, and opting to use
read_ahead_gap for this rather than introduce a new option.

Alex,

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

That would make it inaccurate - there's still one use of
clientStreamInit that does not use this option, in
client_side_request.cc:clientBeginRequest, that works back to
ESISegment::buf, which is a char[HTTP_REQBUF_SZ]. I could convert
ESISegment::buf to a MemBlob and fix the call location in
ESIInclude::Start, but I would not be able to test the code at all, as
I've not used ESI before.

On 4 April 2016 at 10:32, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 03/30/2016 11:50 PM, Nathan Hoad wrote:
>
>> Alex, I've tried 8, 16, 32, 128 and 512 KB values - all sizes leading
>> up to 64 KB scaled appropriately. 128 and 512 were the same or
>> slightly worse than 64, so I think 64 KB is the "best value".
>
> Sounds good, but it is even more important that you have explained *why*
> below.
>
>
>> On 30 March 2016 at 21:29, Amos Jeffries <squid3 at treenet.co.nz> wrote:
>>> One thing you need to keep in mind with all this is that the above
>>> macros *does not* configure the network I/O buffers.
>
>> I don't think this is quite true - I don't think it's intentional, but
>> I am lead to believe that HTTP_REQBUF_SZ does influence network IO
>> buffers in some way. See below.
>
> You are probably both right: HTTP_REQBUF_SZ influences network I/O but
> not necessarily all network I/O buffer capacities. In other words,
> increasing HTTP_REQBUF_SZ improves uncachable miss performance until
> HTTP_REQBUF_SZ matches the size of the second smallest buffer (or I/O
> size) in the buffer chain. In your test, that limit was read_ahead_gap.
>
>
>>> In the long-term plan those internal uses will be replaced by SBuf
>
> ... or MemBlob or something that does not exist yet but uses MemBlob.
>
> SBuf it trying to provide "everything to everybody" and, naturally,
> becomes a bad choice in some situations with strict requirements (e.g.,
> when one has to preserve raw buffer pointer but does not want single
> buffer content ownership).
>
>
>> Looking purely at system calls, it shows the reads from the upstream
>> server are being read in 16 KB chunks, where as writes to the client
>> are done in 4 KB chunks. With the patch, the writes to the client
>> increase to 16 KB, so it appears that HTTP_REQBUF_SZ does influence
>> network IO in this way.
>
> Naturally: One cannot write using 16 KB chunks when the data goes
> through a 4KB buffer. Please note that the position of that 4KB buffer
> in the buffer chain is almost irrelevant -- the weakest link in the
> chain determines "bytes pipelining" or "bytes streaming" speed.
>
>
>
>> However post-patch the improvement is quite
>> substantial, as the write(2) calls are now using the full 64 KB
>> buffer.
>
> This is an important finding! It shows that read_ahead_gap documentation
> lies or at least misleads: Evidently, that directive controls not just
> accumulation of data but [maximum] network read sizes, at least for
> HTTP. Please fix that documentation in your next patch revision.

Done.

>
>
>> At this stage, I'm not entirely sure what the best course of action
>> is. I'm happy to investigate things further, if people have
>> suggestions. read_ahead_gap appears to influence downstream write
>> buffer sizes, at least up to the maximum of HTTP_REQBUF_SZ.
>
> In other words, Squid Client (http.cc and clients/*) is able to give
> Squid Server (client_side*cc and servers/*) the results of a single
> Client network read (i.e., a single read from the origin server of
> peer). The result is copied to the ClientStream StoreIOBuffer. Thus, if
> we do not want to slow Squid down artificially, the StoreIOBuffer
> capacity should match the maximum Client read I/O size.
>
> Do we use that StoreIOBuffer to write(2) to the HTTP client? If not,
> what controls the I/O buffer size for writing to the HTTP client?

We do yes - that buffer makes it through
client_side.cc:clientSocketRecipient, Http::One::Server::handleReply,
and finally Http::Stream::sendBody, where the actual write is
performed.


>
>Http::One::Server::handleReply
>> It would
>> be nice if that buffer size was independently run-time configurable
>
> Actually, it would be nice if the buffer sizes just matched, avoiding
> both artificial slowdown and the need for careful configuration in most
> cases.
>
> Whether configuration is also desirable in some special situations is a
> separate question. If nobody comes forward with such a special
> situation/need, then we may be better off avoiding adding a yet another
> tunable and simply tying ClientStream buffer capacity to the
> read_ahead_gap value. The only reason to add a configuration option
> [that nobody we know needs] would be backward compatibility.

I've opted to remove the configuration option completely, making the
buffer size in Http::Stream obey read_ahead_gap. If such a use case
does come forward, it's trivial to add a configuration option for it.

>
> If backward compatibility is deemed important here, we can add a
> configuration option, but change the _default_ buffer size to track
> read_ahead_gap and, hence, avoid artificial slowdown (at least in v4).
>
>
> How much impact does increasing read_ahead_gap and HTTP_REQBUF_SZ by B
> and G bytes, respectively have on overall Squid memory usage? Is that
> B+G per concurrent transaction, roughly? Are any other buffer capacities
> that depend on those two parameters?
>
> If the increase is B+G then your changes increase Squid RAM footprint by
> C*(60+48) KB where C is the number of concurrent master transactions. A
> Squid dealing with 1000 concurrent transactions would see its RAM usage
> increase by about 100 MB, which is not terrible (and decreased response
> times may reduce the number of concurrent transactions, partially
> mitigating that increase). The commit message (and release notes) should
> disclose the estimated increase though.

Looking at it, yes that is correct - the increase is indeed C*(60+48)
KB. We could (and I have) instead opt to leave the default for
read_ahead_gap at 16kb, at which point the increase is C*12 KB, which
feels nicer to me, with a note in the documentation that increasing
this number may improve throughput at the cost of memory (and should
be tested).

>
>
> Thank you,
>
> Alex.

> AFAICT, reply_header_max_size affects the HTTP response reading only
> until the headers are parsed:
>
>>     const int limitBuffer = (flags.headers_parsed ? Config.readAheadGap : Config.maxReplyHeaderSize);
>
> This is actually kind of wrong because the first read should be
> something like the maximum of those two values: Restricting response
> header size should not restrict response body data prefetch during the
> first I/O. Nathan, if you agree, fixing this would be in your patch
> scope IMO!

That line is... not very nice. It's assigning to an int,
Config.readAheadGap is an int64_t, and Config.maxReplyHeaderSize is
size_t. Making the change you recommended reveals this via a
compilation error:

http.cc: In member function ‘bool HttpStateData::maybeMakeSpaceAvailable(bool)’:
http.cc:1547:125: error: no matching function for call to
‘max(size_t&, int64_t&)’
     const int limitBuffer = (flags.headers_parsed ?
Config.readAheadGap : max(Config.maxReplyHeaderSize,
Config.readAheadGap));

                                                      ^
In file included from ../compat/compat.h:100:0,
                 from ../include/squid.h:43,
                 from http.cc:16:
../compat/compat_shared.h:142:1: note: candidate: template<class A>
const A& max(const A&, const A&)
 max(A const & lhs, A const & rhs)
 ^
../compat/compat_shared.h:142:1: note:   template argument
deduction/substitution failed:
http.cc:1547:125: note:   deduced conflicting types for parameter
‘const A’ (‘long unsigned int’ and ‘int64_t {aka long int}’)
     const int limitBuffer = (flags.headers_parsed ?
Config.readAheadGap : max(Config.maxReplyHeaderSize,
Config.readAheadGap));

I'm happy to include the change - just let me know which way I should
go with this and I'll resubmit.

Thank you,

Nathan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: recv-buffer-v4.patch
Type: text/x-patch
Size: 7336 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160404/a3fa843d/attachment-0001.bin>