rpc: reduce LOCK(cs_min) scope in rest_block: ~5 times as many requests per second

martinus commented at 6:55 pm on January 25, 2021: contributor

In #11281, commit ccd8ef6 the LOCK(cs_main) scope was reduced for the wallet. This does the same for the rest API, which is a huge performance boost for rest_block call when used from multiple threads.

I’m not 100% sure though if this is allowed here…

My test setup, on an Intel i7 with 6 cores (12 threads), locked to 3.2GHz:

start a fully synced bitcoind, with this bitcoin.conf:

0server=1
1rest=1
2rpcport=8332
3rpcthreads=12
4rpcworkqueue=64
5txindex=1
6dbcache=2000

Wait until log message shows loadblk thread exit, so that bitcoind is idle.

Run ApacheBench: 10000 requests, 12 parallel threads, fetching block nr. 600000 in binary:

0ab -n 10000 -c 12 "http://127.0.0.1:8332/rest/block/00000000000000000007316856900e76b4f7a9139cfbfba89842c8d196cd5f91.bin"

Time per request (mean)

97.434 [ms] on master
20.431 [ms] this branch

So this can process 4.8 times as many requests, and saturates all my cores instead of keeping them partly idle waiting in the lock.

rpc: reduce LOCK(cs_min) scope in rest_block

In #11281, commit ccd8ef6 the `LOCK(cs_main)` scope was reduced for
the wallet. This does the same for the rest API, which is a huge
performance boost for `rest_block` call when used from multiple threads.

My test setup, on an Intel i7 with 6 cores (12 threads), locked to 3.2GHz:

1. start a fully synced bitcoind, with this `bitcoin.conf`:
   ```
   server=1
   rest=1
   rpcport=8332
   rpcthreads=12
   rpcworkqueue=64
   txindex=1
   dbcache=2000
   ```
2. Wait until log message shows `loadblk thread exit`, so that bitcoind
   is idle.
3. Run ApacheBench: 10000 requests, 12 parallel threads, fetching block
   nr. 600000 in binary:
   ```
   ab -n 10000 -c 12 "http://127.0.0.1:8332/rest/block/00000000000000000007316856900e76b4f7a9139cfbfba89842c8d196cd5f91.bin"
   ```
Requests per second:
* 97.434 [ms] (mean) on master
* 20.431 [ms] this branch

So this can process about 5 times as many requests, and saturates all my
cores instead of keeping them idle waiting in the lock.

62f1a5b115

DrahtBot added the label RPC/REST/ZMQ on Jan 25, 2021

in src/rest.cpp:259 in 62f1a5b115

253@@ -254,11 +254,11 @@ static bool rest_block(HTTPRequest* req,
254 
255         if (IsBlockPruned(pblockindex))
256             return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
257-
258-        if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))
259-            return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
260     }
261 
262+    if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))

promag commented at 10:01 pm on January 27, 2021:

You need to call with CDiskBlockPos, not with CBlockIndex*.

martinus commented at 6:42 am on January 28, 2021:

I only moved the two lines out of the lock scope, the overload for ReadBlockFromDisk does that (I think CDiskBlockPos was renamed to FlatFilePos in 65a489e93d181d3c0f7a9cf79f7c11ff8cf2b0f0)

jonatack commented at 5:25 pm on September 4, 2021:

(While touching this) since the Apple “goto fail” vulnerability (https://dwheeler.com/essays/apple-goto-fail), we add brackets for conditionals of more than one line

0-    if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus()))
1+    if (!ReadBlockFromDisk(block, pblockindex, Params().GetConsensus())) {
2         return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
3+    }

promag commented at 10:02 pm on January 27, 2021: member

Concept ACK, looks like same approach can be used in other places.

jonatack commented at 9:55 pm on March 16, 2021: contributor

Concept ACK, will review soon.

maflcko commented at 7:21 am on March 17, 2021: member

See also #17161

jonatack commented at 5:33 pm on September 4, 2021: contributor

Code review ACK 62f1a5b115f043a1583849a313b195f05ec602cd

Maybe here in zmqpublishnotifier.cpp as well, IIUC, since the lock is taken in ReadBlockFromDisk()

0bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
1{
2    FlatFilePos blockPos;
3    {
4        LOCK(cs_main);
5        blockPos = pindex->GetBlockPos();
6    }

 0diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp
 1index 6ae866cc07..10893b3291 100644
 2--- a/src/zmq/zmqpublishnotifier.cpp
 3+++ b/src/zmq/zmqpublishnotifier.cpp
 4@@ -206,7 +206,6 @@ bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex)
 5     const Consensus::Params& consensusParams = Params().GetConsensus();
 6     CDataStream ss(SER_NETWORK, PROTOCOL_VERSION | RPCSerializationFlags());
 7     {
 8-        LOCK(cs_main);
 9         CBlock block;
10         if(!ReadBlockFromDisk(block, pindex, consensusParams))

theStack approved

theStack commented at 3:46 pm on October 14, 2021: contributor

Code-review ACK 62f1a5b115f043a1583849a313b195f05ec602cd

Happy to also rereview if you decide to pick up jonatack’s suggestion: #21006 (review)

DrahtBot commented at 3:57 am on May 20, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25168 (refactor: Avoid passing params where not needed by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot commented at 1:41 pm on May 20, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on May 20, 2022

achow101 commented at 6:23 pm on October 12, 2022: member

Are you still working on this?

martinus commented at 5:50 am on October 13, 2022: contributor

@achow101 nope, I don’t have the time/interest any more for this

fanquake closed this on Oct 13, 2022

fanquake added the label Up for grabs on Oct 13, 2022

fanquake removed the label Up for grabs on Oct 14, 2022

fanquake commented at 1:05 am on October 14, 2022: member

Picked up in #26308.

maflcko referenced this in commit 1801d8c3c9 on Dec 8, 2022

bitcoin locked this on Oct 14, 2023

rpc: reduce LOCK(cs_min) scope in rest_block: ~5 times as many requests per second #21006

Conflicts