~cprov/squid/snap

Viewing all changes in revision 14710.

  • Committer: Amos Jeffries
  • Author(s): Alex Rousskov
  • Date: 2016-06-14 16:54:23 UTC
  • Revision ID: squid3@treenet.co.nz-20160614165423-tq5rb30dmfvtiwrs
Fixed Server::maybeMakeSpaceAvailable() logic.

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.

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

expand all expand all

Show diffs side-by-side

added added

removed removed

Lines of Context: