Collection of minor performance optimizations (#2855)

* Merge bitcoin#13176: Improve CRollingBloomFilter performance: replace modulus with FastMod

9aac9f9 replace modulus with FastMod (Martin Ankerl)

Pull request description:

  Not sure if this is optimization is necessary, but anyway I have some spare time so here it is. This replaces the slow modulo operation with a much faster 64bit multiplication & shift. This works when the hash is uniformly distributed between 0 and 2^32-1. This speeds up the benchmark by a factor of about 1.3:

  ```
  RollingBloom, 5, 1500000, 3.73733, 4.97569e-07, 4.99002e-07, 4.98372e-07 # before
  RollingBloom, 5, 1500000, 2.86842, 3.81630e-07, 3.83730e-07, 3.82473e-07 # FastMod
  ```

  Be aware that this changes the internal data of the filter, so this should probably
  not be used for CBloomFilter because of interoperability problems.

Tree-SHA512: 04104f3fb09f56c9d14458a6aad919aeb0a5af944e8ee6a31f00e93c753e22004648c1cd65bf36752b6addec528d19fb665c27b955ce1666a85a928e17afa47a

* Use unordered_map in CSporkManager

In one of my profiling sessions with many InstantSend transactions
happening, calls into CSporkManager added up to about 1% of total CPU time.
This is easily avoidable by using unordered maps.

* Use std::unordered_map instead of std::map in limitedmap

* Use unordered_set for CNode::setAskFor

* Add serialization support for unordered maps and sets

* Use unordered_map for mapArgs and mapMultiArgs

* Let limitedmap prune in batches and use unordered_multimap

Due to the batched pruning, there is no need to maintain an ordered map
of values anymore. Only when nPruneAfterSize, there is a need to create
a temporary ordered vector of values to figure out what can be removed.

* Instead of using a multimap for mapAskFor, use a vector which we sort on demand

CNode::AskFor will now push entries into an initially unordered vector
instead of an ordered multimap. Only when we later want to use vecAskFor in
SendMessages, we sort the vector.

The vector will actually be mostly sorted in most cases as insertion order
usually mimics the desired ordering. Only the last few entries might need
some shuffling around. Doing the sort on-demand should be less wasteful
then trying to maintain correct order all the time.

* Fix compilation of tests

* Fix limitedmap tests

* Rename limitedmap to unordered_limitedmap to ensure backports conflict

This ensures that future backports that depends on limitedmap's ordering
conflict so that we are made aware of needed action.

* Fix compilation error on Travis

Author: codablock

src/bloom.cpp

@@ -331,6 +331,14 @@ static inline uint32_t RollingBloomHash(unsigned int nHashNum, uint32_t nTweak,
     return MurmurHash3(nHashNum * 0xFBA4C795 + nTweak, vDataToHash);
 }
 
+
+// A replacement for x % n. This assumes that x and n are 32bit integers, and x is a uniformly random distributed 32bit value
+// which should be the case for a good hash.
+// See https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/
+static inline uint32_t FastMod(uint32_t x, size_t n) {
+    return ((uint64_t)x * (uint64_t)n) >> 32;
+}
+
 void CRollingBloomFilter::insert(const std::vector<unsigned char>& vKey)
 {
     if (nEntriesThisGeneration == nEntriesPerGeneration) {
@@ -354,7 +362,8 @@ void CRollingBloomFilter::insert(const std::vector<unsigned char>& vKey)
     for (int n = 0; n < nHashFuncs; n++) {
         uint32_t h = RollingBloomHash(n, nTweak, vKey);
         int bit = h & 0x3F;
-        uint32_t pos = (h >> 6) % data.size();
+        /* FastMod works with the upper bits of h, so it is safe to ignore that the lower bits of h are already used for bit. */
+        uint32_t pos = FastMod(h, data.size());
         /* The lowest bit of pos is ignored, and set to zero for the first bit, and to one for the second. */
         data[pos & ~1] = (data[pos & ~1] & ~(((uint64_t)1) << bit)) | ((uint64_t)(nGeneration & 1)) << bit;
         data[pos | 1] = (data[pos | 1] & ~(((uint64_t)1) << bit)) | ((uint64_t)(nGeneration >> 1)) << bit;
@@ -372,7 +381,7 @@ bool CRollingBloomFilter::contains(const std::vector<unsigned char>& vKey) const
     for (int n = 0; n < nHashFuncs; n++) {
         uint32_t h = RollingBloomHash(n, nTweak, vKey);
         int bit = h & 0x3F;
-        uint32_t pos = (h >> 6) % data.size();
+        uint32_t pos = FastMod(h, data.size());
         /* If the relevant bit is not set in either data[pos & ~1] or data[pos | 1], the filter does not contain vKey */
         if (!(((data[pos & ~1] | data[pos | 1]) >> bit) & 1)) {
             return false;

src/limitedmap.h

@@ -6,31 +6,43 @@
 #define BITCOIN_LIMITEDMAP_H
 
 #include <assert.h>
-#include <map>
+#include <algorithm>
+#include <unordered_map>
+#include <vector>
 
 /** STL-like map container that only keeps the N elements with the highest value. */
-template <typename K, typename V>
-class limitedmap
+// WARNING, this was initially the "limitedmap" class from Bitcoin, but now does not maintain ordering. If any backports
+// ever start using this map in a way that requires ordering, do NOT use this as it is but instead reintroduce the original
+// limitedmap
+template <typename K, typename V, typename Hash = std::hash<K>>
+class unordered_limitedmap
 {
 public:
     typedef K key_type;
     typedef V mapped_type;
     typedef std::pair<const key_type, mapped_type> value_type;
-    typedef typename std::map<K, V>::const_iterator const_iterator;
-    typedef typename std::map<K, V>::size_type size_type;
+    typedef typename std::unordered_map<K, V, Hash>::const_iterator const_iterator;
+    typedef typename std::unordered_map<K, V, Hash>::size_type size_type;
 
 protected:
-    std::map<K, V> map;
-    typedef typename std::map<K, V>::iterator iterator;
-    std::multimap<V, iterator> rmap;
-    typedef typename std::multimap<V, iterator>::iterator rmap_iterator;
+    std::unordered_map<K, V, Hash> map;
+    typedef typename std::unordered_map<K, V, Hash>::iterator iterator;
+    std::unordered_multimap<V, iterator> rmap;
+    typedef typename std::unordered_multimap<V, iterator>::iterator rmap_iterator;
     size_type nMaxSize;
+    size_type nPruneAfterSize;
 
 public:
-    limitedmap(size_type nMaxSizeIn)
+    unordered_limitedmap(size_type nMaxSizeIn, size_type nPruneAfterSizeIn = 0)
     {
         assert(nMaxSizeIn > 0);
         nMaxSize = nMaxSizeIn;
+        if (nPruneAfterSizeIn == 0) {
+            nPruneAfterSize = nMaxSize;
+        } else {
+            nPruneAfterSize = nPruneAfterSizeIn;
+        }
+        assert(nPruneAfterSize >= nMaxSize);
     }
     const_iterator begin() const { return map.begin(); }
     const_iterator end() const { return map.end(); }
@@ -42,10 +54,7 @@ class limitedmap
     {
         std::pair<iterator, bool> ret = map.insert(x);
         if (ret.second) {
-            if (map.size() > nMaxSize) {
-                map.erase(rmap.begin()->second);
-                rmap.erase(rmap.begin());
-            }
+            prune();
             rmap.insert(make_pair(x.second, ret.first));
         }
     }
@@ -85,16 +94,43 @@ class limitedmap
         assert(0);
     }
     size_type max_size() const { return nMaxSize; }
-    size_type max_size(size_type s)
+    size_type max_size(size_type nMaxSizeIn, size_type nPruneAfterSizeIn = 0)
     {
-        assert(s > 0);
-        while (map.size() > s) {
-            map.erase(rmap.begin()->second);
-            rmap.erase(rmap.begin());
+        assert(nMaxSizeIn > 0);
+        nMaxSize = nMaxSizeIn;
+        if (nPruneAfterSizeIn == 0) {
+            nPruneAfterSize = nMaxSize;
+        } else {
+            nPruneAfterSize = nPruneAfterSizeIn;
         }
-        nMaxSize = s;
+        assert(nPruneAfterSize >= nMaxSize);
+        prune();
         return nMaxSize;
     }
+    void prune()
+    {
+        if (map.size() <= nPruneAfterSize) {
+            return;
+        }
+
+        std::vector<rmap_iterator> sortedIterators;
+        sortedIterators.reserve(map.size());
+        for (auto it = rmap.begin(); it != rmap.end(); ++it) {
+            sortedIterators.emplace_back(it);
+        }
+        std::sort(sortedIterators.begin(), sortedIterators.end(), [](const rmap_iterator& it1, const rmap_iterator& it2) {
+            return it1->first < it2->first;
+        });
+
+        size_type tooMuch = map.size() - nMaxSize;
+        assert(tooMuch > 0);
+        sortedIterators.resize(tooMuch);
+
+        for (auto& it : sortedIterators) {
+            map.erase(it->second);
+            rmap.erase(it);
+        }
+    }
 };
 
 #endif // BITCOIN_LIMITEDMAP_H

src/net.cpp

@@ -85,7 +85,7 @@ std::map<CNetAddr, LocalServiceInfo> mapLocalHost;
 static bool vfLimited[NET_MAX] = {};
 std::string strSubVersion;
 
-limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);
+unordered_limitedmap<uint256, int64_t, StaticSaltedHasher> mapAlreadyAskedFor(MAX_INV_SZ);
 
 // Signals for message handling
 static CNodeSignals g_signals;
@@ -3142,11 +3142,11 @@ CNode::~CNode()
 
 void CNode::AskFor(const CInv& inv, int64_t doubleRequestDelay)
 {
-    if (mapAskFor.size() > MAPASKFOR_MAX_SZ || setAskFor.size() > SETASKFOR_MAX_SZ) {
+    if (vecAskFor.size() > MAPASKFOR_MAX_SZ || setAskFor.size() > SETASKFOR_MAX_SZ) {
         int64_t nNow = GetTime();
         if(nNow - nLastWarningTime > WARNING_INTERVAL) {
-            LogPrintf("CNode::AskFor -- WARNING: inventory message dropped: mapAskFor.size = %d, setAskFor.size = %d, MAPASKFOR_MAX_SZ = %d, SETASKFOR_MAX_SZ = %d, nSkipped = %d, peer=%d\n",
-                      mapAskFor.size(), setAskFor.size(), MAPASKFOR_MAX_SZ, SETASKFOR_MAX_SZ, nNumWarningsSkipped, id);
+            LogPrintf("CNode::AskFor -- WARNING: inventory message dropped: vecAskFor.size = %d, setAskFor.size = %d, MAPASKFOR_MAX_SZ = %d, SETASKFOR_MAX_SZ = %d, nSkipped = %d, peer=%d\n",
+                      vecAskFor.size(), setAskFor.size(), MAPASKFOR_MAX_SZ, SETASKFOR_MAX_SZ, nNumWarningsSkipped, id);
             nLastWarningTime = nNow;
             nNumWarningsSkipped = 0;
         }
@@ -3159,10 +3159,10 @@ void CNode::AskFor(const CInv& inv, int64_t doubleRequestDelay)
     if (!setAskFor.insert(inv.hash).second)
         return;
 
-    // We're using mapAskFor as a priority queue,
+    // We're using vecAskFor as a priority queue,
     // the key is the earliest time the request can be sent
     int64_t nRequestTime;
-    limitedmap<uint256, int64_t>::const_iterator it = mapAlreadyAskedFor.find(inv.hash);
+    auto it = mapAlreadyAskedFor.find(inv.hash);
     if (it != mapAlreadyAskedFor.end())
         nRequestTime = it->second;
     else
@@ -3183,18 +3183,15 @@ void CNode::AskFor(const CInv& inv, int64_t doubleRequestDelay)
         mapAlreadyAskedFor.update(it, nRequestTime);
     else
         mapAlreadyAskedFor.insert(std::make_pair(inv.hash, nRequestTime));
-    mapAskFor.insert(std::make_pair(nRequestTime, inv));
+    vecAskFor.emplace_back(nRequestTime, inv);
 }
 
 void CNode::RemoveAskFor(const uint256& hash)
 {
-    setAskFor.erase(hash);
-    for (auto it = mapAskFor.begin(); it != mapAskFor.end();) {
-        if (it->second.hash == hash) {
-            it = mapAskFor.erase(it);
-        } else {
-            ++it;
-        }
+    if (setAskFor.erase(hash)) {
+        vecAskFor.erase(std::remove_if(vecAskFor.begin(), vecAskFor.end(), [&](const std::pair<int64_t, CInv>& item) {
+            return item.second.hash == hash;
+        }), vecAskFor.end());
     }
 }
 

src/net.h

@@ -15,6 +15,7 @@
 #include "netaddress.h"
 #include "protocol.h"
 #include "random.h"
+#include "saltedhasher.h"
 #include "streams.h"
 #include "sync.h"
 #include "uint256.h"
@@ -28,6 +29,7 @@
 #include <thread>
 #include <memory>
 #include <condition_variable>
+#include <unordered_set>
 
 #ifndef WIN32
 #include <arpa/inet.h>
@@ -604,7 +606,7 @@ extern bool fDiscover;
 extern bool fListen;
 extern bool fRelayTxes;
 
-extern limitedmap<uint256, int64_t> mapAlreadyAskedFor;
+extern unordered_limitedmap<uint256, int64_t, StaticSaltedHasher> mapAlreadyAskedFor;
 
 /** Subversion as sent to the P2P network in `version` messages */
 extern std::string strSubVersion;
@@ -792,8 +794,8 @@ class CNode
     // List of non-tx/non-block inventory items
     std::vector<CInv> vInventoryOtherToSend;
     CCriticalSection cs_inventory;
-    std::set<uint256> setAskFor;
-    std::multimap<int64_t, CInv> mapAskFor;
+    std::unordered_set<uint256, StaticSaltedHasher> setAskFor;
+    std::vector<std::pair<int64_t, CInv>> vecAskFor;
     int64_t nNextInvSend;
     // Used for headers announcements - unfiltered blocks to relay
     // Also protected by cs_inventory

src/net_processing.cpp

@@ -3732,9 +3732,11 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interr
         //
         // Message: getdata (non-blocks)
         //
-        while (!pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow)
+        std::sort(pto->vecAskFor.begin(), pto->vecAskFor.end());
+        auto it = pto->vecAskFor.begin();
+        while (it != pto->vecAskFor.end() && it->first <= nNow)
         {
-            const CInv& inv = (*pto->mapAskFor.begin()).second;
+            const CInv& inv = it->second;
             if (!AlreadyHave(inv))
             {
                 LogPrint("net", "SendMessages -- GETDATA -- requesting inv = %s peer=%d\n", inv.ToString(), pto->id);
@@ -3750,8 +3752,9 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interr
                 LogPrint("net", "SendMessages -- GETDATA -- already have inv = %s peer=%d\n", inv.ToString(), pto->id);
                 pto->setAskFor.erase(inv.hash);
             }
-            pto->mapAskFor.erase(pto->mapAskFor.begin());
+            ++it;
         }
+        pto->vecAskFor.erase(pto->vecAskFor.begin(), it);
         if (!vGetData.empty()) {
             connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
             LogPrint("net", "SendMessages -- GETDATA -- pushed size = %lu peer=%d\n", vGetData.size(), pto->id);