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

[Alex Rousskov]

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. 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.


Cheers,

Alex.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: make-space-available-t5.patch
Type: text/x-diff
Size: 18622 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160513/c36aefdd/attachment.patch>