From: Alex Rousskov Date: Sat, 18 Jun 2016 13:36:07 +0000 (+1200) Subject: Fixed ConnStateData::In::maybeMakeSpaceAvailable() logic. X-Git-Tag: SQUID_3_5_20~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9b41754498bb692f045ca88556fa6d30a8de8573;p=thirdparty%2Fsquid.git Fixed ConnStateData::In::maybeMakeSpaceAvailable() logic. This change fixes logic bugs that mostly affect performance: In micro- tests, this change gives 10% performance improvement. maybeMakeSpaceAvailable() is called with an essentially random in.buf. The method must prepare in.buf for the next network read. The old code was not doing that [well enough], leading to performance problems. In some environments, in.buf 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 in.bufs "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 in.buf 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 in.buf points to an essentially random MemBlob outside ConnStateData control but this change does not attempt to fix that. --- diff --git a/src/SBuf.cc b/src/SBuf.cc index bea4933951..c8172e86b0 100644 --- a/src/SBuf.cc +++ b/src/SBuf.cc @@ -162,6 +162,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.h b/src/SBuf.h index b9d8d15f21..8a40d2bdc4 100644 --- a/src/SBuf.h +++ b/src/SBuf.h @@ -75,6 +75,7 @@ public: }; class CharacterSet; +class SBufReservationRequirements; /** * A String or Buffer. @@ -424,6 +425,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' @@ -617,6 +624,24 @@ 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; + + SBufReservationRequirements() : idealSpace(0), minSpace(0), maxCapacity(SBuf::maxSize), allowShared(true) {} + + /* + * Parameters are listed in the reverse order of importance: Satisfaction of + * the lower-listed requirements may violate the higher-listed requirements. + */ + size_type idealSpace; ///< if allocating anyway, provide this much space + size_type minSpace; ///< allocate if spaceSize() is smaller + size_type maxCapacity; ///< do not allocate more than this + bool allowShared; ///< 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/client_side.cc b/src/client_side.cc index e05b6a2430..5f17984ffe 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2351,26 +2351,24 @@ parseHttpRequest(ConnStateData *csd, HttpParser *hp, HttpRequestMethod * method_ return result; } -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 ConnStateData::In::maybeMakeSpaceAvailable() { - if (buf.spaceSize() < 2) { - const SBuf::size_type haveCapacity = buf.length() + buf.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. - buf.reserveCapacity(CLIENT_REQ_BUF_SZ); - } else { - const SBuf::size_type wantCapacity = min(static_cast(Config.maxRequestBufferSize), haveCapacity*2); - buf.reserveCapacity(wantCapacity); - } - debugs(33, 2, "growing request buffer: available=" << buf.spaceSize() << " used=" << buf.length()); - } - return (buf.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 + buf.reserve(requirements); + if (!buf.spaceSize()) + debugs(33, 4, "request buffer full: client_request_buffer_max_size=" << Config.maxRequestBufferSize); } void diff --git a/src/client_side.h b/src/client_side.h index 8819f7b2bc..e489e6946f 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -195,10 +195,11 @@ public: // Client TCP connection details from comm layer. Comm::ConnectionPointer clientConnection; - struct In { + class In { + public: In(); ~In(); - bool maybeMakeSpaceAvailable(); + void maybeMakeSpaceAvailable(); ChunkedCodingParser *bodyParser; ///< parses chunked request body SBuf buf; diff --git a/src/tests/stub_SBuf.cc b/src/tests/stub_SBuf.cc index c0f98d3ff7..4342fd25d6 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) diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc index f495251502..708bfa2ea4 100644 --- a/src/tests/stub_client_side.cc +++ b/src/tests/stub_client_side.cc @@ -80,7 +80,7 @@ void ConnStateData::buildSslCertGenerationParams(Ssl::CertificateProperties &cer bool ConnStateData::serveDelayedError(ClientSocketContext *context) STUB_RETVAL(false) #endif -bool ConnStateData::In::maybeMakeSpaceAvailable() STUB_RETVAL(false) +void ConnStateData::In::maybeMakeSpaceAvailable() STUB void setLogUri(ClientHttpRequest * http, char const *uri, bool cleanUrl) STUB const char *findTrailingHTTPVersion(const char *uriAndHTTPVersion, const char *end) STUB_RETVAL(NULL)