[WIP] [mempool] Mempool snapshots to avoid lots of locking #11084

kallewoof commented at 7:07 AM on August 18, 2017: member

This PR addresses the tx relay trickle case specifically, but the snapshot feature can be used in other places as well, where a known set of mempool transactions are being juggled around.

A new class TxMemPoolSnapshot is introduced (63e7997) which is instantiated from CTxMemPool via the snapshot() method. This generates a minimalized, no-locks mempool which can be queried and discarded at will. It is obviously not thread safe.

In 499f14f, this is used for the tx trickle code in SendMessages. This code is executed roughly 5-6 times per second. The number of LOCK(cs) inside the mempool are proportional to the number of entries in vInvTx (because of the sorting part using CompareInvMempoolOrder), which ranges in size from 0 to over 4000.

I began investigating this after seeing 131703419 occurrences of LOCK(cs) in the mempool caused by 504302 entries into the tx relay trickle code path (that's ~250 locks per entry). This was after less than 24 hours of running.

fanquake added the label Mempool on Aug 18, 2017

kallewoof force-pushed on Aug 18, 2017

kallewoof renamed this:
~~[mempool] Mempool snapshots to avoid lots of locking~~
[WIP] [mempool] Mempool snapshots to avoid lots of locking
on Aug 18, 2017

in src/net_processing.cpp:3191 in fa3d8d3712 outdated

3183 | @@ -3183,6 +3184,12 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interr
3184 |                      }
3185 |                      pto->filterInventoryKnown.insert(hash);
3186 |                  }
3187 | +                int64_t nInvTrickleTimeEnd = GetTimeMicros();
3188 | +                nInvTrickleTimeSum += (nInvTrickleTimeEnd - nInvTrickleTimeStart);
3189 | +                nInvTricklePasses++;
3190 | +                if (nInvTricklePasses % 1000 == 0) {
3191 | +                    LogPrint(BCLog::BENCH, "    - inv.trickle: %.2fμs/tx [%.2f txs/pass]\n", (float)nInvTrickleTimeSum / nInvTrickleCount, (float)nInvTrickleCount / nInvTricklePasses);

TheBlueMatt commented at 10:25 PM on August 20, 2017:

Hmm, maybe just "us"? Dunno if we want to introduce unicode into our logs by default.

kallewoof commented at 4:41 AM on August 21, 2017:

Makes sense, yeah.

in src/net_processing.cpp:3131 in d8e8280a4e outdated

3127 |                  vInvTx.reserve(pto->setInventoryTxToSend.size());
3128 | -                for (std::set<uint256>::iterator it = pto->setInventoryTxToSend.begin(); it != pto->setInventoryTxToSend.end(); it++) {
3129 | -                    vInvTx.push_back(it);
3130 | +                for (std::set<uint256>::iterator it = pto->setInventoryTxToSend.begin(); it != pto->setInventoryTxToSend.end(); ) {
3131 | +                    vInvTx.push_back(*it);
3132 | +                    it = pto->setInventoryTxToSend.erase(it);

TheBlueMatt commented at 11:36 PM on August 20, 2017:

I suppose things are broken after commit "[refactor] Use uint256 vector instead of set iterator vector in trick…" but fixed later? Looks like it'll wipe setInventoryTxToSend always, whereas previously it oulw only wipe the first INVENTORY_BROADCAST_MAX entries and retry the rest later, and then the next commit fixes it by limiting this loop to INVENTORY_BROADCAST_MAX.

kallewoof commented at 4:40 AM on August 21, 2017:

Yes, I realized that I broke things temporarily. I will rearrange the commits so that is not the case anymore.

TheBlueMatt commented at 11:45 PM on August 20, 2017: member

Usually nice to put new benchmarks first, to help people benchmark before and after the changeset :).

in src/net_processing.cpp:3129 in fa3d8d3712 outdated

3128 | -                for (std::set<uint256>::iterator it = pto->setInventoryTxToSend.begin(); it != pto->setInventoryTxToSend.end(); it++) {
3129 | -                    vInvTx.push_back(it);
3130 | +                std::vector<uint256> vInvTx;
3131 | +                vInvTx.reserve(std::max<size_t>(pto->setInventoryTxToSend.size(), INVENTORY_BROADCAST_MAX));
3132 | +                size_t count = 0;
3133 | +                for (std::set<uint256>::iterator it = pto->setInventoryTxToSend.begin(); count++ < INVENTORY_BROADCAST_MAX && it != pto->setInventoryTxToSend.end(); ) {

TheBlueMatt commented at 11:53 PM on August 20, 2017:

Any idea how much of your imrpovement is just that you now dont build the heap on the entire set of setInventoryTxToSend as it previously did (also, is that really a good idea? I assume we shouldnt change that?).

kallewoof commented at 4:42 AM on August 21, 2017:

I did profiling over the weekend for "post merge" and am doing profiling for "pre merge" now. Will post back once it's done.

Edit: as for how much is due to not using the entire set, that's a good question. I didn't realize INVENTORY_BROADCAST_MAX was quite as small as it is.

kallewoof commented at 5:46 AM on August 21, 2017:

I reverted to the previous behavior in 4165f43 -- unfortunately need to re-do benchmarks but should have some numbers soon.

kallewoof force-pushed on Aug 21, 2017

[bench] Profile tx relay trickle block. cd290fc4f2

[refactor] Minor clean-up. 9fab9ed2ad

[refactor] Use uint256 vector instead of set iterator vector in trickling tx relay. e007eab01e

[mempool] Add TxMemPoolSnapshot class.

The mempool snapshot feature can be used to fetch a subset of the transactions in the mempool, and to make queries against that without caring about concurrency (the snapshot is single thread use only), saving a bunch of LOCK() calls (millions per day).

8036b79c04

[net] Use mempool snapshot for tx relay trickle. 463080a342

f'swap to vector from set to avoid instantiation 523508d07b

f'revert to using set iterator and remove from setInventoryTxtoSend in while loop 210b236525

kallewoof force-pushed on Aug 21, 2017

kallewoof commented at 7:14 AM on August 21, 2017: member

Closing this until I've gotten some more benchmarks in place (and it's turning out slower than the original currently).

kallewoof closed this on Aug 21, 2017

kallewoof deleted the branch on Oct 17, 2019

DrahtBot locked this on Dec 16, 2021