rpc: Speedup getrawmempool when verbose=true #14984

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-12-speedup-getrawmempool changing 1 files +4 −1

promag commented at 3:36 PM on December 17, 2018: member

Instead of calling pushKV(hash, info), which incurs in duplicate key checks, call __pushKV which (currently) doesn't.

Improves RPC getrawmempool and REST /rest/mempool/contents.json.

Fixes #14765.
fanquake added the label RPC/REST/ZMQ on Dec 17, 2018
promag commented at 5:32 PM on December 17, 2018: member

Thanks @MarcoFalke, the report says >100k. I don't think this matters for small mempool.

promag commented at 9:35 PM on December 17, 2018: member

mempool with 100k transactions:

Before:

time bitcoin-cli -regtest getrawmempool true  > /dev/null
bitcoin-cli -regtest getrawmempool true > /dev/null  3.75s user 0.90s system 8% cpu 52.170 total

After:

time bitcoin-cli -regtest getrawmempool true  > /dev/null
bitcoin-cli -regtest getrawmempool true > /dev/null  3.43s user 0.73s system 45% cpu 9.174 total

I guess you could duplicate one of the benchmarks

Can you point me in the right direction?

or write a functional test.

What should I test?

Empact commented at 7:41 AM on December 18, 2018: member

Could call __pushKV instead.
promag commented at 7:55 AM on December 18, 2018: member

I don't think that's "public"?
Empact commented at 8:14 AM on December 18, 2018: member

It's declared publicly, may be __ because it does not check the type of target, which is set properly here.
promag commented at 10:20 AM on December 18, 2018: member

__pushKV was added in https://github.com/jgarzik/univalue/commit/ceb1194137421afba01e8c1042bf75f3a2cdddc3.

Looks like it could be either private or have another name. I don't have strong preference as the goal is to avoid the duplicate key check, but I'm more inclined to not use the "internal/private/reserved" method.

OTOH it would be reasonable to add duplicate key checks to pushKVs and then we would have a performance regression.

Maybe we should rename __pushKV to pushKVUnchecked?

in src/rpc/blockchain.cpp:471 in 02e0f35116 outdated

 465 | @@ -466,7 +466,10 @@ UniValue mempoolToJSON(bool fVerbose)
 466 |              const uint256& hash = e.GetTx().GetHash();
 467 |              UniValue info(UniValue::VOBJ);
 468 |              entryToJSON(info, e);
 469 | -            o.pushKV(hash.ToString(), info);
 470 | +            UniValue entry(UniValue::VOBJ);
 471 | +            entry.pushKV(hash.ToString(), info);
 472 | +            // `pushKVs` is used to avoid duplicate key check in `pushKV`

MarcoFalke commented at 4:31 PM on February 21, 2019:

            // pushKVs is used to avoid duplicate key check in pushKV
            // It is safe to use because ::mempool.mapTx is a set without duplicates

promag force-pushed on Feb 21, 2019
promag commented at 5:05 PM on February 21, 2019: member

@MarcoFalke not sure about the benchmark. This is a small change to fix the original issue. But I think univalue should have an official API to avoid duplicate key checks and so the benchmark should be there, or am I wrong?

Pushed with your suggestion.
DrahtBot commented at 5:00 PM on February 25, 2019: member



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



Conflicts

No conflicts as of last run.
DrahtBot added the label Needs rebase on Mar 6, 2019
rpc: Speedup getrawmempool when verbose=true
Co-Authored-By: MarcoFalke <falke.marco@gmail.com>
2d5cf4c41d
promag force-pushed on Mar 8, 2019
promag commented at 3:52 PM on March 8, 2019: member

Rebased, switched to UniValue::__pushKV and adjusted the comment.
DrahtBot removed the label Needs rebase on Mar 8, 2019
MarcoFalke referenced this in commit cd14d210c4 on Apr 23, 2019
MarcoFalke added this to the milestone 0.19.0 on May 7, 2019
MarcoFalke added the label Resource usage on May 7, 2019
promag commented at 5:04 PM on May 14, 2019: member

Anything left to do here? This is pretty much the same as #15463.
MarcoFalke referenced this in commit 2d16fb7a2b on May 15, 2019
MarcoFalke merged this on May 15, 2019
MarcoFalke closed this on May 15, 2019
sidhujag referenced this in commit 43e363151b on May 15, 2019

0xB10C commented at 6:34 PM on May 15, 2019: member

I've seen this PR a bit late (just after it got merged), but fwiw here is some real world improvement data:

Time it takes to GET /rest/mempool/contents.json (querys over a minute apart):

in v0.18.0 :

57.9s for 59474 unconfirmed Transactions (36.11 MB)
48.7s for 60199 unconfirmed Transactions (37.02 MB) 
59.2s for 55544 unconfirmed Transactions (36.14 MB) 
45.5s for 55464 unconfirmed Transactions (35.45 MB)
avg. of 52.8s

v0.18.0 with the proposed change:

35.8s for 54308 unconfirmed Transactions (34.94 MB)
40.4s for 55065 unconfirmed Transactions (35.17 MB)
37.0s for 55914 unconfirmed Transactions (35.47 MB)
33.4s for 56739 unconfirmed Transactions (35.73 MB) 
avg. of 36.6s

Which for my sample is a reduction of 16.2s or 30%!! :tada:

promag deleted the branch on May 15, 2019
promag commented at 6:44 PM on May 15, 2019: member

Thanks @0xB10C for the feedback.
promag commented at 6:37 AM on May 16, 2019: member

This is an easy backport but I'm not sure if it's a candidate.
promag commented at 7:35 PM on May 16, 2019: member

@laanwj I'm going to backport this 0.18.1, not sure if milestone should be changed.
fanquake commented at 10:03 PM on May 16, 2019: member

@promag the milestone can remain 0.19.0 here, your backport PR will get 0.18.1.
MarcoFalke commented at 3:57 PM on May 20, 2019: member

https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1&base=1%2B23&ben=micro.clang.RpcMempool&env=1&revs=10&equid=off&quarts=on&extr=on

<img src="https://screenshotscdn.firefoxusercontent.com/images/669c4a3d-b06d-4592-9b78-d9028f248327.png"></img>
promag commented at 7:47 PM on May 23, 2019: member

@0xB10C after all this is not a backport candidate.
deadalnix referenced this in commit e217d0e4cf on May 8, 2020
pravblockc referenced this in commit ef518a1303 on Sep 25, 2021
pravblockc referenced this in commit 9577ca7b25 on Sep 29, 2021
kittywhiskers referenced this in commit 3ee77fcf6e on Oct 12, 2021
dzutto referenced this in commit 0763dad6e7 on Oct 12, 2021
dzutto referenced this in commit 9ffc857c0f on Oct 12, 2021
dzutto referenced this in commit fec569b9ef on Oct 12, 2021
UdjinM6 referenced this in commit 4ac54be71b on Oct 13, 2021
pravblockc referenced this in commit d714af8e31 on Nov 18, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

RPC/REST/ZMQ Resource usage

Milestone
0.19.0

Linked (view graph)

#14765 getrawmempool true RPC call is O(n^2)#15463 rpc: Speedup getaddressesbylabel

rpc: Speedup getrawmempool when verbose=true #14984

Conflicts