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 bea4933951344b806cf93a64235532be9adcdd70..c8172e86b059f12a462f3582bb2608a60e436cfe 100644 (file)
--- 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 b9d8d15f21a5007b5da7d543cef028d723238497..8a40d2bdc4907a819bf0b82e840a2724598fa5b5 100644 (file)
--- 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 e05b6a243058eebe4e7c6926fc2f4fea9814648a..5f17984ffeb2b625e34ed717d236dde3731b7b06 100644 (file)
--- 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<SBuf::size_type>(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 8819f7b2bc88469905ac5037dffb6413da8cf3e4..e489e6946f382a24c587b71c5d4fe3e1a8b65514 100644 (file)
--- 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 c0f98d3ff7b5dcd9f72ab172be3813e1eac312a3..4342fd25d68159ee155f2647b36a0564d313bbd0 100644 (file)
--- 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 f495251502fe0060577617930d9dbbe7f41a552f..708bfa2ea4652ffa6df692234120f80025583ed4 100644 (file)
--- 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)