rpc: Speedup getrawmempool when verbose=true by promag · Pull Request #14984 · bitcoin/bitcoin

promag · 2018-12-17T15:36:44Z

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.

promag · 2018-12-17T17:32:36Z

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

promag · 2018-12-17T21:35:09Z

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 · 2018-12-18T07:41:39Z

Could call __pushKV instead.

promag · 2018-12-18T07:55:43Z

I don't think that's "public"?

Empact · 2018-12-18T08:14:17Z

It's declared publicly, may be __ because it does not check the type of target, which is set properly here.

promag · 2018-12-18T10:20:59Z

__pushKV was added in jgarzik/univalue@ceb1194.

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?

src/rpc/blockchain.cpp

promag · 2019-02-21T17:05:01Z

@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 · 2019-02-25T17:00:08Z

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

Conflicts

No conflicts as of last run.

Co-Authored-By: MarcoFalke <[email protected]>

promag · 2019-03-08T15:52:19Z

Rebased, switched to UniValue::__pushKV and adjusted the comment.

710a713 rpc: Speedup getaddressesbylabel (João Barbosa) Pull request description: Fixes #15447. Same approach of #14984, this change avoids duplicate key check when building the JSON response in memory. ACKs for commit 710a71: MarcoFalke: utACK 710a713 ryanofsky: utACK 710a713. Just new comments and assert since last review. Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2

promag · 2019-05-14T17:04:53Z

Anything left to do here? This is pretty much the same as #15463.

2d5cf4c rpc: Speedup getrawmempool when verbose=true (João Barbosa) Pull request description: 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 bitcoin#14765. ACKs for commit 2d5cf4: Tree-SHA512: c3e91371bb41f39e79dcef820815e1dc27fb689ca3c4bf3a00467d2215b3baecd44d9792f7a481577a5b7ae1fc6cbaa07b1cd62123b845082eba65b35c2b3ca5

0xB10C · 2019-05-15T18:34:57Z

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%!! 🎉

promag · 2019-05-15T18:44:07Z

Thanks @0xB10C for the feedback.

promag · 2019-05-16T06:37:54Z

This is an easy backport but I'm not sure if it's a candidate.

promag · 2019-05-16T19:35:49Z

@laanwj I'm going to backport this 0.18.1, not sure if milestone should be changed.

fanquake · 2019-05-16T22:03:43Z

@promag the milestone can remain 0.19.0 here, your backport PR will get 0.18.1.

maflcko · 2019-05-20T15:57:19Z

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 · 2019-05-23T19:47:29Z

@0xB10C after all this is not a backport candidate.

Summary: 2d5cf4c41d rpc: Speedup getrawmempool when verbose=true (João Barbosa) Pull request description: 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. ACKs for commit 2d5cf4: Tree-SHA512: c3e91371bb41f39e79dcef820815e1dc27fb689ca3c4bf3a00467d2215b3baecd44d9792f7a481577a5b7ae1fc6cbaa07b1cd62123b845082eba65b35c2b3ca5 Backport of Core [[bitcoin/bitcoin#14984 | PR14984]] Related to D5975 Test Plan: Compile two versions of bitcoind, one with this patch and the other without, then record timings for fetching the mempool via REST. ``` git ch master ninja bitcoind bitcoin-cli bitcoind -rest -connect=0 # -connect=0 ensures we do not download any new transactions while testing bitcoin-cli invalidateblock 000000000000000000e311a3b4f53085a427a22dcc8c790b266d9f882c795564 # some block that is not recent bitcoin-cli getmempoolinfo # confirm you have at least thousands of txs in the mempool for X in {1..100} ; do { time curl localhost:8332/rest/mempool/contents.json ; } 2>> results-without-patch ; done git ch <this-patch> ninja bitcoind bitcoin-cli bitcoind -rest -connect=0 # -connect=0 ensures we do not download any new transactions while testing bitcoin-cli getmempoolinfo # confirm you have the same number of transactions in the mempool as the previous test for X in {1..100} ; do { time curl localhost:8332/rest/mempool/contents.json ; } 2>> results-with-patch ; done ``` Now compare the average of the timings: ``` cat results-without-patch | grep real | sed 's/real[ \t]//g' | sed 's/[ms]//g' | awk '{total+=$1; count+=1} END {print total/count}' cat results-with-patch | grep real | sed 's/real[ \t]//g' | sed 's/[ms]//g' | awk '{total+=$1; count+=1} END {print total/count}' ``` My results: Without patch: `0.32937` With patch: `0.17476` **~47% reduction** Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6007

710a713 rpc: Speedup getaddressesbylabel (João Barbosa) Pull request description: Fixes bitcoin#15447. Same approach of bitcoin#14984, this change avoids duplicate key check when building the JSON response in memory. ACKs for commit 710a71: MarcoFalke: utACK 710a713 ryanofsky: utACK 710a713. Just new comments and assert since last review. Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2

2d5cf4c rpc: Speedup getrawmempool when verbose=true (João Barbosa) Pull request description: 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 bitcoin#14765. ACKs for commit 2d5cf4: Tree-SHA512: c3e91371bb41f39e79dcef820815e1dc27fb689ca3c4bf3a00467d2215b3baecd44d9792f7a481577a5b7ae1fc6cbaa07b1cd62123b845082eba65b35c2b3ca5

Merge bitcoin PRs: bitcoin#14683, bitcoin#15841, bitcoin#14984 and bitcoin#15943

2d5cf4c rpc: Speedup getrawmempool when verbose=true (João Barbosa) Pull request description: 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 bitcoin#14765. ACKs for commit 2d5cf4: Tree-SHA512: c3e91371bb41f39e79dcef820815e1dc27fb689ca3c4bf3a00467d2215b3baecd44d9792f7a481577a5b7ae1fc6cbaa07b1cd62123b845082eba65b35c2b3ca5

fanquake added the RPC/REST/ZMQ label Dec 17, 2018

maflcko reviewed Feb 21, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2018-12-speedup-getrawmempool branch from b243d5e to 26b5806 Compare February 21, 2019 17:01

promag mentioned this pull request Feb 22, 2019

rpc: Speedup getaddressesbylabel #15463

Merged

DrahtBot added the Needs rebase label Mar 6, 2019

rpc: Speedup getrawmempool when verbose=true

2d5cf4c

Co-Authored-By: MarcoFalke <[email protected]>

promag force-pushed the 2018-12-speedup-getrawmempool branch from 26b5806 to 2d5cf4c Compare March 8, 2019 15:50

DrahtBot removed the Needs rebase label Mar 8, 2019

maflcko added this to the 0.19.0 milestone May 7, 2019

maflcko added the Resource usage label May 7, 2019

maflcko merged commit 2d5cf4c into bitcoin:master May 15, 2019

promag deleted the 2018-12-speedup-getrawmempool branch May 15, 2019 18:43

fanquake mentioned this pull request May 17, 2019

0.18.1: Backports #16035

Merged

chris-belcher mentioned this pull request May 17, 2019

EPS hangs on get_fee_histogram chris-belcher/electrum-personal-server#96

Closed

UdjinM6 added a commit to dashpay/dash that referenced this pull request Oct 13, 2021

Merge pull request #4514 from dzutte-cpp/merge_14683_15841_14984_15943

4ac54be

Merge bitcoin PRs: bitcoin#14683, bitcoin#15841, bitcoin#14984 and bitcoin#15943

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

promag commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Dec 17, 2018

Uh oh!

promag commented Dec 17, 2018

Uh oh!

Empact commented Dec 18, 2018

Uh oh!

promag commented Dec 18, 2018

Uh oh!

Empact commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

promag commented Feb 21, 2019

Uh oh!

DrahtBot commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Mar 8, 2019

Uh oh!

promag commented May 14, 2019

Uh oh!

0xB10C commented May 15, 2019

Uh oh!

promag commented May 15, 2019

Uh oh!

promag commented May 16, 2019

Uh oh!

promag commented May 16, 2019

Uh oh!

fanquake commented May 16, 2019

Uh oh!

maflcko commented May 20, 2019

Uh oh!

promag commented May 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

promag commented Dec 17, 2018 •

edited

Loading

Empact commented Dec 18, 2018 •

edited

Loading

promag commented Dec 18, 2018 •

edited

Loading

DrahtBot commented Feb 25, 2019 •

edited

Loading