Merge pull request #2771 from hidenori-shinohara/cache-isAQuorum

Cache the results of `isAQuorum`

Reviewed-by: MonsieurNicolas

src/herder/QuorumIntersectionCheckerImpl.cpp

@@ -346,6 +346,7 @@ QuorumIntersectionCheckerImpl::QuorumIntersectionCheckerImpl(
     , mQuiet(quiet)
     , mTSC(mGraph)
     , mInterruptFlag(interruptFlag)
+    , mCachedQuorums(MAX_CACHED_QUORUMS_SIZE)
 {
     buildGraph(qmap);
     buildSCCs();
@@ -464,7 +465,17 @@ QuorumIntersectionCheckerImpl::containsQuorumSliceForNode(BitSet const& bs,
 bool
 QuorumIntersectionCheckerImpl::isAQuorum(BitSet const& nodes) const
 {
-    return (bool)contractToMaximalQuorum(nodes);
+    bool* pRes = mCachedQuorums.maybeGet(nodes);
+    if (pRes == nullptr)
+    {
+        bool result = (bool)contractToMaximalQuorum(nodes);
+        mCachedQuorums.put(nodes, result);
+        return result;
+    }
+    else
+    {
+        return *pRes;
+    }
 }
 
 BitSet
@@ -478,23 +489,23 @@ QuorumIntersectionCheckerImpl::contractToMaximalQuorum(BitSet nodes) const
     }
     while (true)
     {
-        BitSet filtered(nodes.count());
+        BitSet filtered(nodes);
         for (size_t i = 0; nodes.nextSet(i); ++i)
         {
-            if (containsQuorumSliceForNode(nodes, i))
+            if (containsQuorumSliceForNode(filtered, i))
             {
                 if (mLogTrace)
                 {
                     CLOG(TRACE, "SCP") << "Have qslice for " << i;
                 }
-                filtered.set(i);
             }
             else
             {
                 if (mLogTrace)
                 {
                     CLOG(TRACE, "SCP") << "Missing qslice for " << i;
                 }
+                filtered.unset(i);
             }
         }
         if (filtered.count() == nodes.count() || filtered.empty())

src/herder/QuorumIntersectionCheckerImpl.h

@@ -353,6 +353,7 @@
 #include "QuorumIntersectionChecker.h"
 #include "main/Config.h"
 #include "util/BitSet.h"
+#include "util/RandomEvictionCache.h"
 #include "xdr/Stellar-SCP.h"
 #include "xdr/Stellar-types.h"
 
@@ -530,6 +531,10 @@ class QuorumIntersectionCheckerImpl : public stellar::QuorumIntersectionChecker
     bool containsQuorumSlice(BitSet const& bs, QBitSet const& qbs) const;
     bool containsQuorumSliceForNode(BitSet const& bs, size_t node) const;
     BitSet contractToMaximalQuorum(BitSet nodes) const;
+
+    const int MAX_CACHED_QUORUMS_SIZE = 0xffff;
+    mutable stellar::RandomEvictionCache<BitSet, bool, BitSet::HashFunction>
+        mCachedQuorums;
     bool isAQuorum(BitSet const& nodes) const;
     bool isMinimalQuorum(BitSet const& nodes) const;
     void noteFoundDisjointQuorums(BitSet const& nodes,

src/util/BitSet.h

@@ -351,6 +351,25 @@ class BitSet
     {
         streamWith(out, [](std::ostream& out, size_t i) { out << i; });
     }
+    class HashFunction
+    {
+        std::hash<uint64_t> mHasher;
+
+      public:
+        size_t
+        operator()(BitSet const& bitset) const noexcept
+        {
+            // Implementation taken from Boost.
+            // https://www.boost.org/doc/libs/1_35_0/doc/html/boost/hash_combine_id241013.html
+            size_t seed = 0;
+            for (size_t i = 0; i < bitset.mPtr->arraysize; i++)
+            {
+                seed ^= mHasher(bitset.mPtr->array[i]) + 0x9e3779b9 +
+                        (seed << 6) + (seed >> 2);
+            }
+            return seed;
+        }
+    };
 };
 
 inline std::ostream&

src/util/RandomEvictionCache.h

@@ -181,22 +181,37 @@ class RandomEvictionCache : public NonMovableOrCopyable
         }
     }
 
-    // `get` offers strong exception safety guarantee.
-    V&
-    get(K const& k)
+    // `maybeGet` offers basic exception safety guarantee.
+    // Returns a pointer to the value if the key exists,
+    // and returns a nullptr otherwise.
+    V*
+    maybeGet(K const& k)
     {
         auto it = mValueMap.find(k);
         if (it != mValueMap.end())
         {
             auto& cacheVal = it->second;
             ++mCounters.mHits;
             cacheVal.mLastAccess = ++mGeneration;
-            return cacheVal.mValue;
+            return &cacheVal.mValue;
         }
         else
+        {
+            ++mCounters.mMisses;
+            return nullptr;
+        }
+    }
+
+    // `get` offers basic exception safety guarantee.
+    V&
+    get(K const& k)
+    {
+        V* result = maybeGet(k);
+        if (result == nullptr)
         {
             throw std::range_error("There is no such key in cache");
         }
+        return *result;
     }
 };
 }

src/util/test/CacheTests.cpp

@@ -25,9 +25,14 @@ TEST_CASE("cachetable works as a cache", "[randomevictioncache]")
         auto p = cache.get(i);
         REQUIRE(p == i * 100);
     }
+    for (int i = 0; i < sz; ++i)
+    {
+        auto p = *cache.maybeGet(i);
+        REQUIRE(p == i * 100);
+    }
     REQUIRE(ctrs.mInserts == sz);
     REQUIRE(ctrs.mUpdates == 0);
-    REQUIRE(ctrs.mHits == sz);
+    REQUIRE(ctrs.mHits == 2 * sz);
     REQUIRE(ctrs.mMisses == 0);
     REQUIRE(ctrs.mEvicts == 0);
 
@@ -41,9 +46,14 @@ TEST_CASE("cachetable works as a cache", "[randomevictioncache]")
         int p = cache.get(i);
         REQUIRE(p == i * 200);
     }
+    for (int i = 0; i < sz; ++i)
+    {
+        int p = *cache.maybeGet(i);
+        REQUIRE(p == i * 200);
+    }
     REQUIRE(ctrs.mInserts == sz);
     REQUIRE(ctrs.mUpdates == sz);
-    REQUIRE(ctrs.mHits == 2 * sz);
+    REQUIRE(ctrs.mHits == 4 * sz);
     REQUIRE(ctrs.mMisses == 0);
     REQUIRE(ctrs.mEvicts == 0);
 
@@ -54,7 +64,7 @@ TEST_CASE("cachetable works as a cache", "[randomevictioncache]")
     }
     REQUIRE(ctrs.mInserts == sz + sz / 2);
     REQUIRE(ctrs.mUpdates == sz);
-    REQUIRE(ctrs.mHits == 2 * sz);
+    REQUIRE(ctrs.mHits == 4 * sz);
     REQUIRE(ctrs.mMisses == 0);
     REQUIRE(ctrs.mEvicts == sz / 2);
 
@@ -68,9 +78,27 @@ TEST_CASE("cachetable works as a cache", "[randomevictioncache]")
     }
     REQUIRE(ctrs.mInserts == sz + sz / 2);
     REQUIRE(ctrs.mUpdates == sz);
-    REQUIRE(ctrs.mHits >= 2 * sz + sz / 2);
+    REQUIRE(ctrs.mHits >= 4 * sz + sz / 2);
     REQUIRE(ctrs.mMisses <= sz / 2);
     REQUIRE(ctrs.mEvicts == sz / 2);
+
+    // Check that enough un-evicted entries are still there.
+    for (int i = 0; i < sz; ++i)
+    {
+        cache.maybeGet(i);
+    }
+    REQUIRE(ctrs.mInserts == sz + sz / 2);
+    REQUIRE(ctrs.mUpdates == sz);
+    REQUIRE(ctrs.mHits >= 5 * sz);
+    REQUIRE(ctrs.mMisses <= sz);
+    REQUIRE(ctrs.mEvicts == sz / 2);
+
+    // Ensure that maybeGet returns nullptr if and only if
+    // cache.exists return false.
+    for (int i = 0; i < sz; i++)
+    {
+        REQUIRE(cache.exists(i) == (cache.maybeGet(i) != nullptr));
+    }
 }
 
 TEST_CASE("cachetable does not thrash", "[randomevictioncachethrash][!hide]")
@@ -91,13 +119,24 @@ TEST_CASE("cachetable does not thrash", "[randomevictioncachethrash][!hide]")
     // LRU this would be the bad case.
     for (int i = 0; i <= sz; ++i)
     {
-        if (cache.exists(i))
+        if (i % 2 == 0)
         {
-            cache.get(i);
+            if (cache.exists(i))
+            {
+                cache.get(i);
+            }
+            else
+            {
+                cache.put(i, i * 100);
+            }
         }
         else
         {
-            cache.put(i, i * 100);
+            auto p = cache.maybeGet(i);
+            if (p == nullptr)
+            {
+                cache.put(i, i * 100);
+            }
         }
     }
     // These are statistical: it's theoretically possible that a terrible stroke
@@ -172,6 +211,30 @@ TEMPLATE_TEST_CASE("cache keeps last read items", "[cache][template]",
     REQUIRE(existing == 5);
 }
 
+TEMPLATE_TEST_CASE("cache keeps last read items with maybeGet",
+                   "[cache][template]", RandCache)
+{
+    TestType c{5};
+    c.put(0, 0);
+    c.put(1, 1);
+    c.put(2, 2);
+    c.put(3, 3);
+    c.put(4, 4);
+    c.maybeGet(0);
+    c.put(5, 5);
+
+    REQUIRE(c.size() == 5);
+    size_t existing = 0;
+    for (int i = 0; i <= 5; ++i)
+    {
+        if (c.exists(i))
+        {
+            ++existing;
+        }
+    }
+    REQUIRE(existing == 5);
+}
+
 TEMPLATE_TEST_CASE("cache replace element", "[cache][template]", RandCache)
 {
     TestType c{5};
@@ -185,6 +248,18 @@ TEMPLATE_TEST_CASE("cache replace element", "[cache][template]", RandCache)
     REQUIRE(c.get(0) == 3);
     c.put(0, 4);
     REQUIRE(c.get(0) == 4);
+
+    TestType d{5};
+    d.put(0, 0);
+    REQUIRE(*d.maybeGet(0) == 0);
+    d.put(0, 1);
+    REQUIRE(*d.maybeGet(0) == 1);
+    d.put(0, 2);
+    REQUIRE(*d.maybeGet(0) == 2);
+    d.put(0, 3);
+    REQUIRE(*d.maybeGet(0) == 3);
+    d.put(0, 4);
+    REQUIRE(*d.maybeGet(0) == 4);
 }
 
 TEMPLATE_TEST_CASE("cache erase_if removes some nodes", "[cache][template]",