Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Feb 25, 2019

This is used in production (e.g. https://jochen-hoenicke.de/queue/#0,24h), so add a benchmark to avoid making it even slower.

Related:

@maflcko maflcko added the Tests label Feb 25, 2019
@promag
Copy link
Contributor

promag commented Feb 25, 2019

Nice, concept ACK.

Changes to entryToJSON should be in 1st commit?

@maflcko maflcko force-pushed the Mf1902-benchRpcMempool branch from 1aaaaa8 to fa38535 Compare February 25, 2019 15:13
@maflcko
Copy link
Member Author

maflcko commented Feb 25, 2019

Thx. Force pushed with changes correctly split up into the commits

bench_bench_bitcoin_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(EVENT_CLFAGS) $(EVENT_PTHREADS_CFLAGS) -I$(builddir)/bench/
bench_bench_bitcoin_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
bench_bench_bitcoin_LDADD = \
$(LIBBITCOIN_SERVER) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this is the first bench touching RPC functionality.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 25, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15323 (rpc: Expose g_is_mempool_loaded via getmempoolinfo by Empact)
  • #14984 (rpc: Speedup getrawmempool when verbose=true by promag)

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.

@practicalswift
Copy link
Contributor

Concept ACK

@maflcko
Copy link
Member Author

maflcko commented Feb 27, 2019

Anyone an idea how to fix the appveyor linker error?

cc @ken2812221

@jgarzik
Copy link
Contributor

jgarzik commented Feb 27, 2019

concept ACK

Comments trigged by this PR, but not directly related:

  1. Recently generated univalue profiling data showed 80% of the CPU time being spent in the destructor, when testing on a large JSON dataset (https://github.com/zemirco/sf-city-lots-json)

I would infer similar behavior in getrawmempool return values.

  1. Would be nice to introduce RPCv2 using protobufs a la lnd. Clearly defined schema, automatic schema validation vs data, and dense data encoding.

@ken2812221
Copy link
Contributor

Seems that bench_bitcoin does not have univalue include path. I created PR #15503 to solve it.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa38535. Nice cleanup and new benchmark.

@maflcko maflcko merged commit fa38535 into bitcoin:master Mar 6, 2019
maflcko pushed a commit that referenced this pull request Mar 6, 2019
fa38535 bench: Benchmark MempoolToJSON (MarcoFalke)
fa5dc35 rpc: Pass mempool into MempoolToJSON (MarcoFalke)

Pull request description:

  This is used in production (e.g. https://jochen-hoenicke.de/queue/#0,24h), so add a benchmark to avoid making it even slower.

  Related:

  * "getrawmempool true RPC call is O(n^2)" #14765

Tree-SHA512: da09d2e54ee261af8671152f97f863cf1acd7a6adc6578e94046b1ec9e647a670c67499760ef765254f65522dfdf773c3c8729006fa2d63ccb6d53166bafc425
@maflcko maflcko deleted the Mf1902-benchRpcMempool branch March 6, 2019 22:00
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 25, 2019
Summary:
This is used in production (e.g. https://jochen-hoenicke.de/queue/#0,24h), so add a benchmark to avoid making it even slower.

Backport of Bitcoin Core PR15473
bitcoin/bitcoin#15473

Test Plan:
1. Build with Clang in Debug mode:

```
CXX=clang++ CC=clang cmake .. -D CMAKE_CXX_FLAGS="-Werror=thread-safety-analysis" -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check-all
```

2. Verify that the compiler has not emitted a `thread-safety` warning.
3. Run the node: `./src/bitcoind -regtest`
4. Verify that text similar to `"Assertion failed: lock ... not held ..."` is not printed on `stderr`.

5. Run benchmarks:
```
cmake .. -GNinja
ninja bench-bitcoin
./src/bench/bitcoin-bench
```

Reviewers: Fabien, #bitcoin_abc, deadalnix

Reviewed By: Fabien, #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D4234
kwvg added a commit to kwvg/dash that referenced this pull request Oct 25, 2021
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants