Optimize freeMemorySpace() to not loop in vain

src/MemObject.cc

@@ -94,7 +94,7 @@ MemObject::setUris(char const *aStoreId, char const *aLogUri, const HttpRequestM
 #endif
 }
 
-MemObject::MemObject()
+MemObject::MemObject(const bool locked) : data_hdr(locked)
 {
     debugs(20, 3, "MemObject constructed, this=" << this);
     ping_reply_callback = nullptr;
@@ -132,19 +132,6 @@ MemObject::replaceBaseReply(const HttpReplyPointer &r)
     updatedReply_ = nullptr;
 }
 
-void
-MemObject::write(const StoreIOBuffer &writeBuffer)
-{
-    debugs(19, 6, "memWrite: offset " << writeBuffer.offset << " len " << writeBuffer.length);
-
-    /* We don't separate out mime headers yet, so ensure that the first
-     * write is at offset 0 - where they start
-     */
-    assert (data_hdr.endOffset() || writeBuffer.offset == 0);
-
-    assert (data_hdr.write (writeBuffer));
-}
-
 void
 MemObject::dump() const
 {

src/MemObject.h

@@ -37,7 +37,8 @@ class MemObject
     static size_t inUseCount();
 
     void dump() const;
-    MemObject();
+    /// \param locked whether the associated StoreEntry is locked
+    MemObject(bool locked);
     ~MemObject();
 
     /// Sets store ID, log URI, and request method (unless already set). Does
@@ -51,7 +52,6 @@ class MemObject
     /// whether setUris() has been called
     bool hasUris() const;
 
-    void write(const StoreIOBuffer &buf);
     void unlinkRequest() { request = nullptr; }
 
     /// HTTP response before 304 (Not Modified) updates

src/MemStore.cc

@@ -569,7 +569,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof)
 
     // local memory stores both headers and body so copy regardless of pstate
     const int64_t offBefore = e.mem_obj->endOffset();
-    assert(e.mem_obj->data_hdr.write(buf)); // from MemObject::write()
+    e.writeData(buf);
     const int64_t offAfter = e.mem_obj->endOffset();
     // expect to write the entire buf because StoreEntry::write() never fails
     assert(offAfter >= 0 && offBefore <= offAfter &&

src/Store.h

@@ -56,6 +56,11 @@ class StoreEntry : public hash_link, public Packable
     /// \see MemObject::freshestReply()
     const HttpReply *hasFreshestReply() const { return mem_obj ? &mem_obj->freshestReply() : nullptr; }
 
+    /// writes to local memory store
+    void writeData(StoreIOBuffer);
+
+    /// writeData() and calls awaiting handlers.
+    /// Should be called for non-completed (STORE_PENDING) entries only.
     void write(StoreIOBuffer);
 
     /** Check if the Store entry is empty
@@ -67,6 +72,9 @@ class StoreEntry : public hash_link, public Packable
     bool isAccepting() const;
     size_t bytesWanted(Range<size_t> const aRange, bool ignoreDelayPool = false) const;
 
+    /// whether a non-completed (STORE_PENDING) entry is being written to the local memory store
+    bool isLocalWriter() const { return locked() && isAccepting(); }
+
     /// Signals that the entire response has been stored and no more append()
     /// calls should be expected; cf. completeTruncated().
     void completeSuccessfully(const char *whyWeAreSureWeStoredTheWholeReply);
@@ -319,6 +327,9 @@ class StoreEntry : public hash_link, public Packable
     /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it
     void lengthWentBad(const char *reason);
 
+    /// whether methods such as lock()/unlock() also require mem_hdr::IdleNodes counter updating
+    bool needsIdlePagesUpdating() const { return !lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL); }
+
     static Mem::Allocator *pool;
 
     unsigned short lock_count;      /* Assume < 65536! */

src/stmem.cc

@@ -15,6 +15,8 @@
 #include "MemObject.h"
 #include "stmem.h"
 
+size_t mem_hdr::IdleNodes = 0;
+
 /*
  * NodeGet() is called to get the data buffer to pass to storeIOWrite().
  * By setting the write_pending flag here we are assuming that there
@@ -58,7 +60,9 @@ mem_hdr::endOffset () const
 void
 mem_hdr::freeContent()
 {
+    const auto initialNodes = size();
     nodes.destroy();
+    updateIdleNodes(initialNodes);
     inmem_hi = 0;
     debugs(19, 9, this << " hi: " << inmem_hi);
 }
@@ -72,8 +76,10 @@ mem_hdr::unlink(mem_node *aNode)
     }
 
     debugs(19, 8, this << " removing " << aNode);
+    const auto initialNodes = size();
     nodes.remove (aNode, NodeCompare);
     delete aNode;
+    updateIdleNodes(initialNodes);
     return true;
 }
 
@@ -302,7 +308,7 @@ mem_hdr::nodeToRecieve(int64_t offset)
 }
 
 bool
-mem_hdr::write (StoreIOBuffer const &writeBuffer)
+mem_hdr::write(const StoreIOBuffer &writeBuffer)
 {
     debugs(19, 6, "mem_hdr::write: " << this << " " << writeBuffer.range() << " object end " << endOffset());
 
@@ -331,7 +337,7 @@ mem_hdr::write (StoreIOBuffer const &writeBuffer)
     return true;
 }
 
-mem_hdr::mem_hdr() : inmem_hi(0)
+mem_hdr::mem_hdr(const bool locked) : inmem_hi(0), isIdle(!locked)
 {
     debugs(19, 9, this << " hi: " << inmem_hi);
 }
@@ -376,3 +382,31 @@ mem_hdr::getNodes() const
     return nodes;
 }
 
+void
+mem_hdr::setIdleness(const bool idle)
+{
+    assert(idle != isIdle);
+    isIdle = idle;
+    if (isIdle) {
+        IdleNodes += size();
+    } else {
+        assert(IdleNodes >= size());
+        IdleNodes -= size();
+    }
+}
+
+void
+mem_hdr::updateIdleNodes(const size_t oldSize)
+{
+    if (!isIdle)
+        return;
+    const auto newSize = size();
+    const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize;
+    if (newSize > oldSize) {
+        IdleNodes += delta;
+    } else if (newSize < oldSize) {
+        assert(IdleNodes >= delta);
+        IdleNodes -= delta;
+    }
+}
+

src/stmem.h

@@ -20,19 +20,26 @@ class mem_hdr
 {
 
 public:
-    mem_hdr();
+    /// \param locked whether the associated StoreEntry is locked
+    mem_hdr(bool locked);
     ~mem_hdr();
     void freeContent();
     int64_t lowestOffset () const;
     int64_t endOffset () const;
     int64_t freeDataUpto (int64_t);
     ssize_t copy (StoreIOBuffer const &) const;
     bool hasContigousContentRange(Range<int64_t> const &range) const;
-    /* success or fail */
+    /// Saves the buffer into the internal storage.
+    /// Do not call directly - use StoreEntry::writeData() instead.
     bool write (StoreIOBuffer const &);
     void dump() const;
     size_t size() const;
     mem_node *getBlockContainingLocation (int64_t location) const;
+    /// switches the 'idleness' status of or all nodes
+    void setIdleness(bool idle);
+    /// Adjusts IdleNodes counter by the difference
+    /// between the current size() and oldSize.
+    void updateIdleNodes(const size_t oldSize);
     /* access the contained nodes - easier than punning
      * as a container ourselves
      */
@@ -41,6 +48,9 @@ class mem_hdr
 
     static Splay<mem_node *>::SPLAYCMP NodeCompare;
 
+    /// the total number of pages belonging to unlocked StoreEntries
+    static size_t IdleNodes;
+
 private:
     void debugDump() const;
     bool unlink(mem_node *aNode);
@@ -51,6 +61,7 @@ class mem_hdr
     size_t writeAvailable(mem_node *aNode, int64_t location, size_t amount, char const *source);
     int64_t inmem_hi;
     Splay<mem_node *> nodes;
+    bool isIdle; ///< whether the associated pages belong to an unlocked StoreEntry
 };
 
 #endif /* SQUID_STMEM_H */

src/store.cc

@@ -418,6 +418,8 @@ StoreEntry::hashDelete()
 void
 StoreEntry::lock(const char *context)
 {
+    if (needsIdlePagesUpdating())
+        mem_obj->data_hdr.setIdleness(false);
     ++lock_count;
     debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this);
 }
@@ -450,6 +452,9 @@ StoreEntry::unlock(const char *context)
     if (lock_count)
         return (int) lock_count;
 
+    if (needsIdlePagesUpdating())
+        mem_obj->data_hdr.setIdleness(true);
+
     abandon(context);
     return 0;
 }
@@ -755,14 +760,14 @@ StoreEntry::write (StoreIOBuffer writeBuffer)
 {
     assert(mem_obj != nullptr);
     /* This assert will change when we teach the store to update */
-    assert(store_status == STORE_PENDING);
+    assert(isLocalWriter());
 
     // XXX: caller uses content offset, but we also store headers
     writeBuffer.offset += mem_obj->baseReply().hdr_sz;
 
     debugs(20, 5, "storeWrite: writing " << writeBuffer.length << " bytes for '" << getMD5Text() << "'");
     storeGetMemSpace(writeBuffer.length);
-    mem_obj->write(writeBuffer);
+    writeData(writeBuffer);
 
     if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT) && !mem_obj->readAheadPolicyCanRead()) {
         debugs(20, 3, "allow Store clients to get entry content after buffering too much for " << *this);
@@ -772,6 +777,19 @@ StoreEntry::write (StoreIOBuffer writeBuffer)
     invokeHandlers();
 }
 
+void
+StoreEntry::writeData(StoreIOBuffer writeBuffer)
+{
+    assert(mem_obj);
+    debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length);
+    auto &dataHdr = mem_obj->data_hdr;
+    assert (dataHdr.endOffset() || writeBuffer.offset == 0);
+    const auto oldSize = dataHdr.size();
+    assert(dataHdr.write(writeBuffer));
+    if (needsIdlePagesUpdating())
+        dataHdr.updateIdleNodes(oldSize);
+}
+
 /* Append incoming data from a primary server to an entry. */
 void
 StoreEntry::append(char const *buf, int len)
@@ -1543,7 +1561,7 @@ void
 StoreEntry::createMemObject()
 {
     assert(!mem_obj);
-    mem_obj = new MemObject();
+    mem_obj = new MemObject(locked());
 }
 
 void
@@ -1557,7 +1575,7 @@ void
 StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod)
 {
     if (!mem_obj)
-        mem_obj = new MemObject();
+        createMemObject();
     mem_obj->setUris(aUrl, aLogUrl, aMethod);
 }
 

src/store/Controller.cc

@@ -339,7 +339,7 @@ Store::Controller::checkFoundCandidate(const StoreEntry &entry) const
             // Transients do check when they setCollapsingRequirement().
         } else {
             // a local writer must hold a lock on its writable entry
-            if (!(entry.locked() && entry.isAccepting()))
+            if (!(entry.isLocalWriter()))
                 throw TextException("no local writer", Here());
         }
     }
@@ -534,6 +534,9 @@ Store::Controller::freeMemorySpace(const int bytesRequired)
     if (memoryCacheHasSpaceFor(pagesRequired))
         return;
 
+    if (mem_hdr::IdleNodes < static_cast<size_t>(pagesRequired))
+        return;
+
     // XXX: When store_pages_max is smaller than pagesRequired, we should not
     // look for more space (but we do because we want to abandon idle entries?).
 

src/store_client.cc

@@ -539,7 +539,7 @@ store_client::readBody(const char *, ssize_t len)
              * copyInto.offset includes headers, which is what mem cache needs
              */
             if (copyInto.offset == entry->mem_obj->endOffset()) {
-                entry->mem_obj->write(StoreIOBuffer(len, copyInto.offset, copyInto.data));
+                entry->writeData(StoreIOBuffer(len, copyInto.offset, copyInto.data));
             }
         }
     }

src/tests/stub_MemObject.cc

@@ -29,7 +29,7 @@ MemObject::endOffset() const
 void MemObject::trimSwappable() STUB
 void MemObject::trimUnSwappable() STUB
 int64_t MemObject::policyLowestOffsetToKeep(bool) const STUB_RETVAL(-1)
-MemObject::MemObject() {
+MemObject::MemObject(bool locked) : data_hdr(locked) {
     ping_reply_callback = nullptr;
     memset(&start_ping, 0, sizeof(start_ping));
 } // NOP instead of elided due to Store
@@ -46,7 +46,6 @@ int MemObject::mostBytesWanted(int, bool) const STUB_RETVAL(-1)
 #if USE_DELAY_POOLS
 DelayId MemObject::mostBytesAllowed() const STUB_RETVAL(DelayId())
 #endif
-void MemObject::write(const StoreIOBuffer &) STUB
 int64_t MemObject::lowestMemReaderOffset() const STUB_RETVAL(0)
 void MemObject::kickReads() STUB
 int64_t MemObject::objectBytesOnDisk() const STUB_RETVAL(0)

src/tests/stub_stmem.cc

@@ -12,7 +12,7 @@
 #define STUB_API "stmem.cc"
 #include "tests/STUB.h"
 
-mem_hdr::mem_hdr() STUB
+mem_hdr::mem_hdr(bool) STUB
 mem_hdr::~mem_hdr() STUB
 size_t mem_hdr::size() const STUB_RETVAL(0)
 int64_t mem_hdr::endOffset () const STUB_RETVAL(0)

src/tests/testStore.cc

@@ -112,6 +112,7 @@ TestStore::testStats()
     Store::Init(aStore);
     CPPUNIT_ASSERT_EQUAL(false, aStore->statsCalled);
     StoreEntry entry;
+    entry.lock("TestStore::testStats");
     Store::Stats(&entry);
     CPPUNIT_ASSERT_EQUAL(true, aStore->statsCalled);
     Store::FreeMemory();

src/tests/testStoreController.cc

@@ -30,6 +30,7 @@ TestStoreController::testStats()
 {
     Store::Init();
     StoreEntry *logEntry = new StoreEntry;
+    logEntry->lock("TestStoreController::testStats");
     logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod());
     logEntry->store_status = STORE_PENDING;
     TestSwapDirPointer aStore (new TestSwapDir);

src/tests/testStoreHashIndex.cc

@@ -29,6 +29,7 @@ void
 TestStoreHashIndex::testStats()
 {
     StoreEntry *logEntry = new StoreEntry;
+    logEntry->lock("TestStoreHashIndex::testStats");
     logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod());
     logEntry->store_status = STORE_PENDING;
     Store::Init();

test-suite/mem_hdr_test.cc

@@ -19,7 +19,7 @@
 static void
 testLowAndHigh()
 {
-    mem_hdr aHeader;
+    mem_hdr aHeader(true);
     assert (aHeader.lowestOffset() == 0);
     assert (aHeader.write (StoreIOBuffer()));
     assert (aHeader.lowestOffset() == 0);
@@ -74,7 +74,7 @@ testSplayOfNodes()
 static void
 testHdrVisit()
 {
-    mem_hdr aHeader;
+    mem_hdr aHeader(true);
     char * sampleData = xstrdup ("A");
     assert (aHeader.write (StoreIOBuffer(1, 100, sampleData)));
     safe_free (sampleData);