------------------------------------------------------------ revno: 14061 revision-id: squid3@treenet.co.nz-20160618133607-yt9vr8gjdrctcqd1 parent: squid3@treenet.co.nz-20160618114803-m7riuy90mrdlxw1f author: Alex Rousskov committer: Amos Jeffries branch nick: 3.5 timestamp: Sun 2016-06-19 01:36:07 +1200 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20160618133607-yt9vr8gjdrctcqd1 # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 86f65c860a5470008ab241b44a72d4cd35418e73 # timestamp: 2016-06-18 13:50:58 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20160618114803-\ # m7riuy90mrdlxw1f # # Begin patch === modified file 'src/SBuf.cc' --- src/SBuf.cc 2016-01-01 00:14:27 +0000 +++ src/SBuf.cc 2016-06-18 13:36:07 +0000 @@ -162,6 +162,29 @@ 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) { === modified file 'src/SBuf.h' --- src/SBuf.h 2016-01-01 00:14:27 +0000 +++ src/SBuf.h 2016-06-18 13:36:07 +0000 @@ -75,6 +75,7 @@ }; class CharacterSet; +class SBufReservationRequirements; /** * A String or Buffer. @@ -424,6 +425,12 @@ */ 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 @@ 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) === modified file 'src/client_side.cc' --- src/client_side.cc 2016-06-15 22:08:16 +0000 +++ src/client_side.cc 2016-06-18 13:36:07 +0000 @@ -2351,26 +2351,24 @@ 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 === modified file 'src/client_side.h' --- src/client_side.h 2016-01-01 00:14:27 +0000 +++ src/client_side.h 2016-06-18 13:36:07 +0000 @@ -195,10 +195,11 @@ // 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; === modified file 'src/tests/stub_SBuf.cc' --- src/tests/stub_SBuf.cc 2016-01-01 00:14:27 +0000 +++ src/tests/stub_SBuf.cc 2016-06-18 13:36:07 +0000 @@ -53,6 +53,7 @@ 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) === modified file 'src/tests/stub_client_side.cc' --- src/tests/stub_client_side.cc 2016-01-01 00:14:27 +0000 +++ src/tests/stub_client_side.cc 2016-06-18 13:36:07 +0000 @@ -80,7 +80,7 @@ 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)