Optimize freeMemorySpace() to not loop in vain

src/MemObject.cc

@@ -100,6 +100,7 @@ MemObject::MemObject()
     ping_reply_callback = nullptr;
     memset(&start_ping, 0, sizeof(start_ping));
     reply_ = new HttpReply;
+    assert(!repl.data);
 }
 
 MemObject::~MemObject()

src/stmem.cc

@@ -13,8 +13,11 @@
 #include "HttpReply.h"
 #include "mem_node.h"
 #include "MemObject.h"
+#include "SquidMath.h"
 #include "stmem.h"
 
+size_t mem_hdr::ReplPolicyIdleNodesCount = 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 +61,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 +77,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;
 }
 
@@ -320,18 +327,20 @@ mem_hdr::write (StoreIOBuffer const &writeBuffer)
     char *currentSource = writeBuffer.data;
     size_t len = writeBuffer.length;
 
+    const auto initialNodes = size();
     while (len && (target = nodeToRecieve(currentOffset))) {
         size_t wrote = writeAvailable(target, currentOffset, len, currentSource);
         assert (wrote);
         len -= wrote;
         currentOffset += wrote;
         currentSource += wrote;
     }
+    updateIdleNodes(initialNodes);
 
     return true;
 }
 
-mem_hdr::mem_hdr() : inmem_hi(0)
+mem_hdr::mem_hdr() : inmem_hi(0), removableByReplPolicy(false)
 {
     debugs(19, 9, this << " hi: " << inmem_hi);
 }
@@ -376,3 +385,40 @@ mem_hdr::getNodes() const
     return nodes;
 }
 
+static void
+UpdateIdleNodesCounter(const size_t oldSize, const size_t newSize)
+{
+    if (newSize == oldSize)
+        return;
+    const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize;
+    const auto oldCount = mem_hdr::ReplPolicyIdleNodesCount;
+    if (newSize > oldSize) {
+        mem_hdr::ReplPolicyIdleNodesCount = IncreaseSum(mem_hdr::ReplPolicyIdleNodesCount, delta).value();
+    } else {
+        assert(mem_hdr::ReplPolicyIdleNodesCount >= delta);
+        mem_hdr::ReplPolicyIdleNodesCount -= delta;
+    }
+    debugs(19, 5, "Updated ReplPolicyIdleNodesCount from " << oldCount << " to " << mem_hdr::ReplPolicyIdleNodesCount);
+}
+
+void
+mem_hdr::allowedToFreeWithReplPolicy(const bool allowed)
+{
+    if (removableByReplPolicy == allowed)
+        return;
+    removableByReplPolicy = allowed;
+    const auto oldSize = removableByReplPolicy ? 0 : size();
+    const auto newSize = removableByReplPolicy ? size() : 0;
+    UpdateIdleNodesCounter(oldSize, newSize);
+}
+
+/// Adjusts the ReplPolicyIdleNodesCount counter by the difference
+/// between the current size() and oldSize.
+void
+mem_hdr::updateIdleNodes(const size_t oldSize)
+{
+    if (!removableByReplPolicy)
+        return;
+    UpdateIdleNodesCounter(oldSize, size());
+}
+

src/stmem.h

@@ -33,6 +33,7 @@ class mem_hdr
     void dump() const;
     size_t size() const;
     mem_node *getBlockContainingLocation (int64_t location) const;
+    void allowedToFreeWithReplPolicy(const bool allowed);
     /* access the contained nodes - easier than punning
      * as a container ourselves
      */
@@ -41,6 +42,10 @@ class mem_hdr
 
     static Splay<mem_node *>::SPLAYCMP NodeCompare;
 
+    /// the total number of pages that allowed to be purged by
+    /// the associated replacement policy
+    static size_t ReplPolicyIdleNodesCount;
+
 private:
     void debugDump() const;
     bool unlink(mem_node *aNode);
@@ -49,8 +54,10 @@ class mem_hdr
     bool unionNotEmpty (StoreIOBuffer const &);
     mem_node *nodeToRecieve(int64_t offset);
     size_t writeAvailable(mem_node *aNode, int64_t location, size_t amount, char const *source);
+    void updateIdleNodes(const size_t oldSize);
     int64_t inmem_hi;
     Splay<mem_node *> nodes;
+    bool removableByReplPolicy; ///< whether nodes allowed to be purged by the associated replacement policy
 };
 
 #endif /* SQUID_STMEM_H */

src/store.cc

@@ -419,6 +419,8 @@ void
 StoreEntry::lock(const char *context)
 {
     ++lock_count;
+    if (mem_obj)
+        mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);
     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 (mem_obj)
+        mem_obj->data_hdr.allowedToFreeWithReplPolicy(mem_obj->repl.data);
+
     abandon(context);
     return 0;
 }
@@ -756,6 +761,7 @@ 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(locked());
 
     // XXX: caller uses content offset, but we also store headers
     writeBuffer.offset += mem_obj->baseReply().hdr_sz;
@@ -1513,13 +1519,16 @@ StoreEntry::setMemStatus(mem_status_t new_status)
         } else {
             mem_policy->Add(mem_policy, this, &mem_obj->repl);
             debugs(20, 4, "inserted " << *this << " key: " << getMD5Text());
+            // only idle entries can be freed by the replacement policy
+            mem_obj->data_hdr.allowedToFreeWithReplPolicy(!locked());
         }
 
         ++hot_obj_count; // TODO: maintain for the shared hot cache as well
     } else {
         if (EBIT_TEST(flags, ENTRY_SPECIAL)) {
             debugs(20, 4, "not removing special " << *this << " from policy");
         } else {
+            mem_obj->data_hdr.allowedToFreeWithReplPolicy(false);
             mem_policy->Remove(mem_policy, this, &mem_obj->repl);
             debugs(20, 4, "removed " << *this);
         }

src/store/Controller.cc

@@ -534,6 +534,10 @@ Store::Controller::freeMemorySpace(const int bytesRequired)
     if (memoryCacheHasSpaceFor(pagesRequired))
         return;
 
+    // do not free anything if freeing everything freeable would not be enough
+    if (Less(mem_hdr::ReplPolicyIdleNodesCount, 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/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();