rpc: Speedup getrawmempool when verbose=true

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:

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

After:

0time bitcoin-cli -regtest getrawmempool true  > /dev/null
1bitcoin-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:

0            // pushKVs is used to avoid duplicate key check in pushKV
1            // 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 :

057.9s for 59474 unconfirmed Transactions (36.11 MB)
148.7s for 60199 unconfirmed Transactions (37.02 MB) 
259.2s for 55544 unconfirmed Transactions (36.14 MB) 
345.5s for 55464 unconfirmed Transactions (35.45 MB)
4avg. of 52.8s

v0.18.0 with the proposed change:

035.8s for 54308 unconfirmed Transactions (34.94 MB)
140.4s for 55065 unconfirmed Transactions (35.17 MB)
237.0s for 55914 unconfirmed Transactions (35.47 MB)
333.4s for 56739 unconfirmed Transactions (35.73 MB) 
4avg. 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

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

rpc: Speedup getrawmempool when verbose=true #14984

Conflicts