[squid-dev] [PATCH] Fix maybeMakeSpaceAvailable() logic

[Amos Jeffries]

On 14/05/2016 9:00 a.m., Alex Rousskov wrote:
> Hello,
> 
>     This change fixes logic bugs that mostly affect performance: In
> micro-tests, this change gives 10% performance improvement for
> intercepted "fast peek at SNI and splice" SslBump configurations.
> Similar improvement is expected for future plain HTTP/2 parsers.
> 
>       trunk  fsni fsni+
>   SS1  100%  100%  100%
>   SS2   22%   69%   77%
>   SS3   16%   26%   26%
> 
> The table format was described in my follow up to Christos' "Fast SNI
> peek" email a few minutes ago. The testing methodology was the same so
> the same caveats apply. The last column contains results from the Fast
> SNI branch code patched with the attached patch.
> 
> 
> Server::maybeMakeSpaceAvailable() is called with an essentially random
> inBuf. The method must prepare inBuf for the next network read. The old
> code was not doing that [well enough], leading to performance problems.
> 
> In some environments, inBuf often ends up having a tiny space exceeding
> 2 bytes (e.g., 6 bytes). This happens, for example, when Squid creates
> and parses a fake CONNECT request. The old code often left such tiny
> inBufs "as is" because we tried to ensure that we have at least 2 bytes
> to read instead of trying to provide a reasonable number of buffer space
> for the next network read. Tiny buffers naturally result in tiny network
> reads, which are very inefficient, especially for non-incremental parsers.
> 
> I have removed the explicit "2 byte" space checks: Both the new and the
> old code do not _guarantee_ that at least 2 bytes of buffer space are
> always available, and the caller does not check that condition either.
> If some other code relies on it, more fixes will be needed (but this
> change is not breaking that guarantee -- either it was broken earlier or
> was never fully enforced). In practice, only buffers approaching
> Config.maxRequestBufferSize limit may violate this guarantee AFAICT, and
> those buffers ought to be rare, so the bug, if any, remains unnoticed.
> 
> Another subtle maybeMakeSpaceAvailable() problem was that the code
> contained its own buffer capacity increase algorithm (n^2 growth).
> However, increasing buffer capacity exponentially does not make much
> sense because network read sizes are not going to increase
> exponentially. Also, memAllocStringmemAllocate() overwrites n^2 growth
> with its own logic. Besides, it is buffer _space_, not the total
> capacity that should be increased.

This last sentence is not right. The point of having buffer limits is
that the buffer must not grow *capacity* past those limits. Capacity can
be partially used resulting in less space available.

I think you mean MemBlob capacity is not strictly aligned with buffer
size limits. Since the MemBlob may contain data from previous reads
which has been processed and no longer considered "in the buffer".

What we need is that the SBuf used as a buffer should guarantee (space +
length) <= limit, regardless of the underlying MemBlob capacity. Without
that guarantee we end up with each read cycle simply growing the
buffered data towards infinity as it consumes "space".


> More work is needed to better match
> Squid buffer size for from-user network reads with the TCP stack buffers
> and traffic patterns.
> 
> Both the old and the new code reallocate inBuf MemBlobs. However, the
> new code leaves "reallocate or memmove" decision to the new
> SBuf::reserve(), opening the possibility for future memmove
> optimizations that SBuf/MemBlob do not currently support.
> 
> It is probably wrong that inBuf points to an essentially random MemBlob
> outside Server control but this change does not attempt to fix that.
> 
> The patch was written and tested ~2 weeks ago so it may need some
> polishing to apply to the current trunk.
> 

I dont think there has been any code drift in this area of trunk. The
following updates need to be made though:


in src/sbuf/SBuf.h:

* member initializers are one of the C++11 features GCC 4.8 has trouble
with.
 - if we still want to offer GCC 4.8 support then this will have to
become a default constructor.
 - I am okay with dropping that support if you want. The release-4.sgml
notes will need to update to remove the "GCC-4.8 will also build for now
..." statement.


in src/sbuf/SBuf.h:

* "if (!mustRealloc && len_ >= req.maxCapacity)" looks wrong.
 - thats igoring how much empty space it currently present
 - capacity overflow for the SBuf (not MemBlob) is:
    length() + spaceSize() >= req.maxCapacity


* likewise the min() calculations are wrong.
 - It will increasingly over-allocate the bigger the buffer actually
gets. For non-limited SBuf that could be very dangerous in the
int-overflow kind of way.
 - Should be:
     wantedSpace = std::min(req.idealSpace, req.maxCapacity - length());
     reserveC..(std::min(length() +wantedSpace, maxSize - length()));

* might have to redo the performance metrics after these to have
accurate numbers. But I dont expect this will be worse than the current
trunk, so up to you.


in src/servers/Server.cc:

* comment reason for "allowShared = true" is wrong. It is allowed
because conceptually this SBuf is the base I/O buffer the caller 'owns'
and all others sharing are just referencing sub-sections of that.
 - you might be able to phrase that better though.


* now that reserve returns spaceSize() you can simplify the pattern:
    inBuf.reserve(requirements);
    if (!inBuf.spaceSize())
 to just:
    if (!inBuf.reserve(requirements))


in src/tests/stub_SBuf.cc:

 * s/reserveCapacity/reserve/  (only for the added line.)


Amos