]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Fixed Server::maybeMakeSpaceAvailable() logic.
authorAlex Rousskov <rousskov@measurement-factory.com>
Tue, 14 Jun 2016 16:54:23 +0000 (04:54 +1200)
committerAmos Jeffries <squid3@treenet.co.nz>
Tue, 14 Jun 2016 16:54:23 +0000 (04:54 +1200)
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
src/sbuf/SBuf.h
src/sbuf/forward.h
src/servers/Server.cc
src/servers/Server.h
src/tests/stub_SBuf.cc

index 647767e4891f8d436098aba995fd260a0e0886f4..929dc157ba0cc8caab3b2305d76be4d9edd76e49 100644 (file)
@@ -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)
 {
index bf14b3dd0b49140ead4bca494b495e3c783ac66c..db64501c1b10f9d573abce9efedf3c0aa60be91b 100644 (file)
@@ -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)
index dea611458a36da8d0a31dd03e7c229bd2ca601d5..d34f6d0274d23439cd10f13882527cf36fe2894c 100644 (file)
@@ -17,6 +17,7 @@ class MemBlob;
 class SBuf;
 class SBufIterator;
 class SBufReverseIterator;
+class SBufReservationRequirements;
 
 class OutOfBoundsException;
 class InvalidParamException;
index ef86a4ddea9058d12c4c9596344cba7d087f0540..d554f30315f6ba521ad914ff87c1ac3e09ebcbef 100644 (file)
@@ -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<SBuf::size_type>(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
index 0d2832fc039424f2f35b5b89eedca1b40697b492..481ce5758f9f312b954715ca4ca0be175ab443fe 100644 (file)
@@ -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;
index a5de68ead671c67b4baabf7fef0c6bcc41f05269..1e7b6c7142d629d12c22326fb253e34cff481a83 100644 (file)
@@ -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)