From 19e97cf09d35e1d35a083f5dc8b1a299d06e2a37 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 15 Jun 2016 04:54:23 +1200 Subject: [PATCH] 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. --- src/sbuf/SBuf.cc | 23 +++++++++++++++++++++++ src/sbuf/SBuf.h | 24 +++++++++++++++++++++++- src/sbuf/forward.h | 1 + src/servers/Server.cc | 34 ++++++++++++++++------------------ src/servers/Server.h | 2 +- src/tests/stub_SBuf.cc | 1 + 6 files changed, 65 insertions(+), 20 deletions(-) diff --git a/src/sbuf/SBuf.cc b/src/sbuf/SBuf.cc index 647767e489..929dc157ba 100644 --- a/src/sbuf/SBuf.cc +++ b/src/sbuf/SBuf.cc @@ -123,6 +123,29 @@ SBuf::reserveCapacity(size_type minCapacity) cow(minCapacity); } +SBuf::size_type +SBuf::reserve(const SBufReservationRequirements &req) +{ + debugs(24, 8, id << " was: " << off_ << '+' << len_ << '+' << spaceSize() << + '=' << store_->capacity); + + const bool mustRealloc = !req.allowShared && store_->LockCount() > 1; + + if (!mustRealloc && spaceSize() >= req.minSpace) + return spaceSize(); // the caller is content with what we have + + /* only reallocation can make the caller happy */ + + if (!mustRealloc && len_ >= req.maxCapacity) + return spaceSize(); // but we cannot reallocate + + const size_type newSpace = std::min(req.idealSpace, maxSize - len_); + reserveCapacity(std::min(len_ + newSpace, req.maxCapacity)); + debugs(24, 7, id << " now: " << off_ << '+' << len_ << '+' << spaceSize() << + '=' << store_->capacity); + return spaceSize(); // reallocated and probably reserved enough space +} + char * SBuf::rawSpace(size_type minSpace) { diff --git a/src/sbuf/SBuf.h b/src/sbuf/SBuf.h index bf14b3dd0b..db64501c1b 100644 --- a/src/sbuf/SBuf.h +++ b/src/sbuf/SBuf.h @@ -15,6 +15,7 @@ #include "Debug.h" #include "globals.h" #include "sbuf/Exceptions.h" +#include "sbuf/forward.h" #include "sbuf/MemBlob.h" #include "sbuf/Stats.h" @@ -39,7 +40,6 @@ typedef enum { } SBufCaseSensitive; class CharacterSet; -class SBuf; /** Forward input const_iterator for SBufs * @@ -465,6 +465,12 @@ public: */ void reserveCapacity(size_type minCapacity); + /** Accommodate caller's requirements regarding SBuf's storage if possible. + * + * \return spaceSize(), which may be zero + */ + size_type reserve(const SBufReservationRequirements &requirements); + /** slicing method * * Removes SBuf prefix and suffix, leaving a sequence of 'n' @@ -686,6 +692,22 @@ private: SBuf& lowAppend(const char * memArea, size_type areaSize); }; +/// Named SBuf::reserve() parameters. Defaults ask for and restrict nothing. +class SBufReservationRequirements +{ +public: + typedef SBuf::size_type size_type; + + /* + * Parameters are listed in the reverse order of importance: Satisfaction of + * the lower-listed requirements may violate the higher-listed requirements. + */ + size_type idealSpace = 0; ///< if allocating anyway, provide this much space + size_type minSpace = 0; ///< allocate if spaceSize() is smaller + size_type maxCapacity = SBuf::maxSize; ///< do not allocate more than this + bool allowShared = true; ///< whether sharing our storage with others is OK +}; + /// ostream output operator inline std::ostream & operator <<(std::ostream& os, const SBuf& S) diff --git a/src/sbuf/forward.h b/src/sbuf/forward.h index dea611458a..d34f6d0274 100644 --- a/src/sbuf/forward.h +++ b/src/sbuf/forward.h @@ -17,6 +17,7 @@ class MemBlob; class SBuf; class SBufIterator; class SBufReverseIterator; +class SBufReservationRequirements; class OutOfBoundsException; class InvalidParamException; diff --git a/src/servers/Server.cc b/src/servers/Server.cc index ef86a4ddea..d554f30315 100644 --- a/src/servers/Server.cc +++ b/src/servers/Server.cc @@ -61,26 +61,24 @@ Server::stopReading() } } -bool +/// Prepare inBuf for I/O. This method balances several conflicting desires: +/// 1. Do not read too few bytes at a time. +/// 2. Do not waste too much buffer space. +/// 3. Do not [re]allocate or memmove the buffer too much. +/// 4. Obey Config.maxRequestBufferSize limit. +void Server::maybeMakeSpaceAvailable() { - if (inBuf.spaceSize() < 2) { - const SBuf::size_type haveCapacity = inBuf.length() + inBuf.spaceSize(); - if (haveCapacity >= Config.maxRequestBufferSize) { - debugs(33, 4, "request buffer full: client_request_buffer_max_size=" << Config.maxRequestBufferSize); - return false; - } - if (haveCapacity == 0) { - // haveCapacity is based on the SBuf visible window of the MemBlob buffer, which may fill up. - // at which point bump the buffer back to default. This allocates a new MemBlob with any un-parsed bytes. - inBuf.reserveCapacity(CLIENT_REQ_BUF_SZ); - } else { - const SBuf::size_type wantCapacity = min(static_cast(Config.maxRequestBufferSize), haveCapacity*2); - inBuf.reserveCapacity(wantCapacity); - } - debugs(33, 2, "growing request buffer: available=" << inBuf.spaceSize() << " used=" << inBuf.length()); - } - return (inBuf.spaceSize() >= 2); + // The hard-coded parameters are arbitrary but seem reasonable. + // A careful study of Squid I/O and parsing patterns is needed to tune them. + SBufReservationRequirements requirements; + requirements.minSpace = 1024; // smaller I/Os are not worth their overhead + requirements.idealSpace = CLIENT_REQ_BUF_SZ; // we expect few larger I/Os + requirements.maxCapacity = Config.maxRequestBufferSize; + requirements.allowShared = true; // allow because inBuf is used immediately + inBuf.reserve(requirements); + if (!inBuf.spaceSize()) + debugs(33, 4, "request buffer full: client_request_buffer_max_size=" << Config.maxRequestBufferSize); } void diff --git a/src/servers/Server.h b/src/servers/Server.h index 0d2832fc03..481ce5758f 100644 --- a/src/servers/Server.h +++ b/src/servers/Server.h @@ -90,7 +90,7 @@ public: public: /// grows the available read buffer space (if possible) - bool maybeMakeSpaceAvailable(); + void maybeMakeSpaceAvailable(); // Client TCP connection details from comm layer. Comm::ConnectionPointer clientConnection; diff --git a/src/tests/stub_SBuf.cc b/src/tests/stub_SBuf.cc index a5de68ead6..1e7b6c7142 100644 --- a/src/tests/stub_SBuf.cc +++ b/src/tests/stub_SBuf.cc @@ -53,6 +53,7 @@ char *SBuf::rawSpace(size_type minSize) STUB_RETVAL(NULL) void SBuf::forceSize(size_type newSize) STUB const char* SBuf::c_str() STUB_RETVAL("") void SBuf::reserveCapacity(size_type minCapacity) STUB +SBuf::size_type SBuf::reserve(const SBufReservationRequirements &) STUB_RETVAL(0) SBuf& SBuf::chop(size_type pos, size_type n) STUB_RETVAL(*this) SBuf& SBuf::trim(const SBuf &toRemove, bool atBeginning, bool atEnd) STUB_RETVAL(*this) SBuf SBuf::substr(size_type pos, size_type n) const STUB_RETVAL(*this) -- 2.47.2