node cache: remove expired weak pointers from cache

Remove expired Node weak pointers regularly,
in a way that ensures that:
* every expired pointer gets eventually cleaned up
* expired pointer ratio remains reasonably low
* no cleanup pass blocks the node for too long,
* impact on performance is negligible

Impact on performance:
Every N insertions, at most N nodes are tested by iterating the map
-> performance impact in O(1) (constant).

include/opendht/node_cache.h

@@ -40,12 +40,16 @@ struct NodeCache {
     ~NodeCache();
 
 private:
-    class NodeMap : public std::map<InfoHash, std::weak_ptr<Node>> {
+    class NodeMap : private std::map<InfoHash, std::weak_ptr<Node>> {
     public:
         Sp<Node> getNode(const InfoHash& id);
         Sp<Node> getNode(const InfoHash& id, const SockAddr&, time_point now, bool confirmed, bool client);
+        std::vector<Sp<Node>> getCachedNodes(const InfoHash& id, size_t count) const;
         void clearBadNodes();
         void setExpired();
+        void cleanup();
+    private:
+        size_t cleanup_counter {0};
     };
 
     const NodeMap& cache(sa_family_t af) const { return af == AF_INET ? cache_4 : cache_6; }

src/node_cache.cpp

@@ -20,6 +20,9 @@
 
 namespace dht {
 
+constexpr size_t CLEANUP_MAX_NODES {1024};
+constexpr size_t CLEANUP_FREQ {1024};
+
 NodeCache::~NodeCache()
 {
     cache_4.setExpired();
@@ -41,28 +44,32 @@ NodeCache::getNode(const InfoHash& id, const SockAddr& addr, time_point now, boo
 std::vector<Sp<Node>>
 NodeCache::getCachedNodes(const InfoHash& id, sa_family_t sa_f, size_t count) const
 {
-    const auto& c = cache(sa_f);
+    return cache(sa_f).getCachedNodes(id, count);
+}
 
+std::vector<Sp<Node>>
+NodeCache::NodeMap::getCachedNodes(const InfoHash& id, size_t count) const
+{
     std::vector<Sp<Node>> nodes;
-    nodes.reserve(std::min(c.size(), count));
-    NodeMap::const_iterator it;
-    auto dec_it = [&c](NodeMap::const_iterator& it) {
+    nodes.reserve(std::min(size(), count));
+    const_iterator it;
+    auto dec_it = [this](const_iterator& it) {
         auto ret = it;
-        it = (it == c.cbegin()) ? c.cend() : std::prev(it);
+        it = (it == cbegin()) ? cend() : std::prev(it);
         return ret;
     };
 
-    auto it_p = c.lower_bound(id),
+    auto it_p = lower_bound(id),
         it_n = it_p;
-    if (not c.empty())
+    if (not empty())
         dec_it(it_p); /* Create 2 separate iterator if we could */
 
-    while (nodes.size() < count and (it_n != c.cend() or it_p != c.cend())) {
+    while (nodes.size() < count and (it_n != cend() or it_p != cend())) {
         /* If one of the iterator is at the end, then take the other one
            If they are both in middle of somewhere comapre both and take
            the closest to the id. */
-        if (it_p == c.cend())       it = it_n++;
-        else if (it_n == c.cend())  it = dec_it(it_p);
+        if (it_p == cend())      it = it_n++;
+        else if (it_n == cend()) it = dec_it(it_p);
         else                     it = id.xorCmp(it_p->first, it_n->first) < 0 ? dec_it(it_p) : it_n++;
 
         if (auto n = it->second.lock())
@@ -104,6 +111,10 @@ NodeCache::NodeMap::getNode(const InfoHash& id, const SockAddr& addr, time_point
     if (not node) {
         node = std::make_shared<Node>(id, addr, client);
         it.first->second = node;
+        if (cleanup_counter++ == CLEANUP_FREQ) {
+            cleanup();
+            cleanup_counter = 0;
+        }
     } else if (confirm or node->isOld(now)) {
         node->update(addr);
     }
@@ -120,6 +131,7 @@ NodeCache::NodeMap::clearBadNodes() {
             erase(it++);
         }
     }
+    cleanup_counter = 0;
 }
 
 void
@@ -128,6 +140,21 @@ NodeCache::NodeMap::setExpired() {
         if (auto n = wn.second.lock())
             n->setExpired();
     clear();
+    cleanup_counter = 0;
+}
+
+void
+NodeCache::NodeMap::cleanup()
+{
+    auto it = lower_bound(InfoHash::getRandom());
+    for (size_t n = 0, maxNodes = std::min(size(), CLEANUP_MAX_NODES); n != maxNodes; n++) {
+        if (it == end())
+            it = begin();
+        if (it->second.expired())
+            erase(it++);
+        else
+            ++it;
+    }
 }
 
 }