Merge pull request #13410 from omoerbeek/rec-cache-tidy

rec: tidy cache and only copy values if non-expired entry was found

pdns/cachecleaner.hh

@@ -130,8 +130,8 @@ uint64_t pruneLockedCollectionsVector(std::vector<T>& maps)
   return totErased;
 }
 
-template <typename S, typename C, typename T>
-uint64_t pruneMutexCollectionsVector(time_t now, C& container, std::vector<T>& maps, uint64_t maxCached, uint64_t cacheSize)
+template <typename S, typename T>
+uint64_t pruneMutexCollectionsVector(time_t now, std::vector<T>& maps, uint64_t maxCached, uint64_t cacheSize)
 {
   uint64_t totErased = 0;
   uint64_t toTrim = 0;
@@ -163,7 +163,7 @@ uint64_t pruneMutexCollectionsVector(time_t now, C& container, std::vector<T>& m
     uint64_t lookedAt = 0;
     for (auto i = sidx.begin(); i != sidx.end(); lookedAt++) {
       if (i->isStale(now)) {
-        container.preRemoval(*shard, *i);
+        shard->preRemoval(*i);
         i = sidx.erase(i);
         erased++;
         content.decEntriesCount();
@@ -223,7 +223,7 @@ uint64_t pruneMutexCollectionsVector(time_t now, C& container, std::vector<T>& m
     auto& sidx = boost::multi_index::get<S>(shard->d_map);
     size_t removed = 0;
     for (auto i = sidx.begin(); i != sidx.end() && removed < toTrimForThisShard; removed++) {
-      container.preRemoval(*shard, *i);
+      shard->preRemoval(*i);
       i = sidx.erase(i);
       content.decEntriesCount();
       ++totErased;

pdns/iputils.hh

@@ -1257,7 +1257,7 @@ public:
   }
 
   //<! checks whether the container is empty.
-  bool empty() const {
+  [[nodiscard]] bool empty() const {
     return (d_size == 0);
   }
 

pdns/recursordist/negcache.cc

@@ -272,7 +272,7 @@ void NegCache::clear()
 void NegCache::prune(time_t now, size_t maxEntries)
 {
   size_t cacheSize = size();
-  pruneMutexCollectionsVector<SequenceTag>(now, *this, d_maps, maxEntries, cacheSize);
+  pruneMutexCollectionsVector<SequenceTag>(now, d_maps, maxEntries, cacheSize);
 }
 
 /*!

pdns/recursordist/negcache.hh

@@ -140,6 +140,7 @@ private:
       uint64_t d_contended_count{0};
       uint64_t d_acquired_count{0};
       void invalidate() {}
+      void preRemoval(const NegCacheEntry& /* entry */) {}
     };
 
     LockGuardedTryHolder<MapCombo::LockedContent> lock()
@@ -188,7 +189,4 @@ private:
   {
     return d_maps.at(qname.hash() % d_maps.size());
   }
-
-public:
-  void preRemoval(MapCombo::LockedContent& /* map */, const NegCacheEntry& /* entry */) {}
 };

pdns/recursordist/pdns_recursor.cc

@@ -1842,10 +1842,10 @@ void startDoResolve(void* arg) // NOLINT(readability-function-cognitive-complexi
 
     if (comboWriter->d_mdp.d_header.opcode == static_cast<unsigned>(Opcode::Query)) {
       if (resolver.d_outqueries != 0 || resolver.d_authzonequeries != 0) {
-        g_recCache->cacheMisses++;
+        g_recCache->incCacheMisses();
       }
       else {
-        g_recCache->cacheHits++;
+        g_recCache->incCacheHits();
       }
     }
 

pdns/recursordist/rec-main.cc

@@ -1072,8 +1072,8 @@ static void doStats()
   static time_t lastOutputTime;
   static uint64_t lastQueryCount;
 
-  uint64_t cacheHits = g_recCache->cacheHits;
-  uint64_t cacheMisses = g_recCache->cacheMisses;
+  uint64_t cacheHits = g_recCache->getCacheHits();
+  uint64_t cacheMisses = g_recCache->getCacheMisses();
   uint64_t cacheSize = g_recCache->size();
   auto rc_stats = g_recCache->stats();
   auto pc_stats = g_packetCache ? g_packetCache->stats() : std::pair<uint64_t, uint64_t>{0, 0};

pdns/recursordist/rec_channel_rec.cc

@@ -1102,16 +1102,6 @@ static uint64_t doGetCacheBytes()
   return g_recCache->bytes();
 }
 
-static uint64_t doGetCacheHits()
-{
-  return g_recCache->cacheHits;
-}
-
-static uint64_t doGetCacheMisses()
-{
-  return g_recCache->cacheMisses;
-}
-
 static uint64_t doGetMallocated()
 {
   // this turned out to be broken
@@ -1291,8 +1281,8 @@ static void registerAllStats1()
   addGetStat("ipv6-questions", [] { return g_Counters.sum(rec::Counter::ipv6qcounter); });
   addGetStat("tcp-questions", [] { return g_Counters.sum(rec::Counter::tcpqcounter); });
 
-  addGetStat("cache-hits", doGetCacheHits);
-  addGetStat("cache-misses", doGetCacheMisses);
+  addGetStat("cache-hits", []() { return g_recCache->getCacheHits(); });
+  addGetStat("cache-misses", []() { return g_recCache->getCacheMisses(); });
   addGetStat("cache-entries", doGetCacheSize);
   addGetStat("max-cache-entries", []() { return g_maxCacheEntries.load(); });
   addGetStat("max-packetcache-entries", []() { return g_maxPacketCacheEntries.load(); });

pdns/recursordist/recpacketcache.cc

@@ -254,7 +254,7 @@ void RecursorPacketCache::insertResponsePacket(unsigned int tag, uint32_t qhash,
 void RecursorPacketCache::doPruneTo(time_t now, size_t maxSize)
 {
   size_t cacheSize = size();
-  pruneMutexCollectionsVector<SequencedTag>(now, *this, d_maps, maxSize, cacheSize);
+  pruneMutexCollectionsVector<SequencedTag>(now, d_maps, maxSize, cacheSize);
 }
 
 uint64_t RecursorPacketCache::doDump(int file)

pdns/recursordist/recpacketcache.hh

@@ -183,6 +183,7 @@ private:
       uint64_t d_contended_count{0};
       uint64_t d_acquired_count{0};
       void invalidate() {}
+      void preRemoval(const Entry& /* entry */) {}
     };
 
     LockGuardedTryHolder<MapCombo::LockedContent> lock()
@@ -241,9 +242,4 @@ private:
   static bool checkResponseMatches(MapCombo::LockedContent& shard, std::pair<packetCache_t::index<HashTag>::type::iterator, packetCache_t::index<HashTag>::type::iterator> range, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, vState* valState, OptPBData* pbdata);
 
   void setShardSizes(size_t shardSize);
-
-public:
-  void preRemoval(MapCombo::LockedContent& /* map */, const Entry& /* entry */)
-  {
-  }
 };