Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Use upstream rocksdb #11248

Merged
merged 18 commits into from
Dec 3, 2019
Merged

Use upstream rocksdb #11248

merged 18 commits into from
Dec 3, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Nov 11, 2019

@dvdplm dvdplm self-assigned this Nov 11, 2019
@dvdplm dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. F5-task 🎬 Generic label for things that just have to get done but are no issues in first place. M5-dependencies 🖇 Dependencies. labels Nov 11, 2019
dvdplm and others added 4 commits November 12, 2019 18:30
* master:
  Clarify what first_block `None` means (#11269)
  removed redundant VMType enum with one variant (#11266)
  Ensure jsonrpc threading settings are sane (#11267)
  Return Ok(None) when the registrar contract returns empty slice (#11257)
  Add a benchmark for snapshot::account::to_fat_rlps() (#11185)
  Fix misc compile warnings (#11258)
  simplify verification (#11249)
@grbIzl
Copy link
Collaborator

grbIzl commented Nov 21, 2019

(Sorry for the stupid question) Do we have any metric or tests for it? I mean, do we understand, how will this affect client's performance? @ordian

@dvdplm
Copy link
Collaborator Author

dvdplm commented Nov 21, 2019

Do we have any metric or tests for it?

Sort of. Both me and @ordian have been running block import tests over the last few days. Here are mine from last night, importing 5k blocks from approx mid-chain:

Master:

Benchmark #1: ./parities/parity-master --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     320.989 s ± 24.305 s    [User: 341.816 s, System: 41.915 s]
  Range (min … max):   302.979 s … 362.882 s    5 runs   

Upstream rocksdb:

Benchmark #1: ./parities/parity-ao --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
   Time (mean ± σ):     283.643 s ± 10.213 s    [User: 282.681 s, System: 43.740 s]
   Range (min … max):   271.713 s … 296.393 s    5 runs

I think @ordian's results are similar.

@ordian
Copy link
Collaborator

ordian commented Nov 21, 2019

@grbIzl I'm seeing 20% improvement on parity import when switching to this branch (tested on 100 and 5k recent blocks from mainnet at ~8.9m).

@dvdplm
Copy link
Collaborator Author

dvdplm commented Nov 21, 2019

More benchmarks, now also comparing a master build from today (Nov 21, 2019), after the new trie landed (referred to below as "fast trie", but it's actually a little slower, cc @cheme @jimpo):

Upstream rocksdb

Benchmark #1: ./parities/parity-ao --mode=offline --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     284.806 s ±  7.885 s    [User: 283.799 s, System: 42.353 s]
  Range (min … max):   277.414 s … 296.277 s    5 runs

Upstream rocksdb + fast trie

Benchmark #1: ./parities/parity-ao-fast-trie --mode=offline --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     292.625 s ± 12.209 s    [User: 290.013 s, System: 42.446 s]
  Range (min … max):   282.579 s … 312.988 s    5 runs

Forked rocksdb (master, Nov 20)

Benchmark #1: ./parities/parity-master --mode=offline --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     321.283 s ±  8.235 s    [User: 340.501 s, System: 41.963 s]
  Range (min … max):   312.589 s … 333.771 s    5 runs

Forked rocksdb + fast trie (master, Nov 21)

Benchmark #1: ./parities/parity-master-fast-trie --mode=offline --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     323.682 s ±  7.001 s    [User: 341.980 s, System: 40.867 s]
  Range (min … max):   317.452 s … 334.709 s    5 runs

I also ran the same set of benchmarks with more verifier threads (8), but it has little impact.

Upstream rocksdb + 8 verif threads

Benchmark #1: ./parities/parity-ao --mode=offline --scale-verifiers --num-verifiers=8 --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     277.217 s ±  8.206 s    [User: 266.063 s, System: 41.339 s]
  Range (min … max):   268.896 s … 288.652 s    5 runs

Upstream rocksdb + fast trie + 8 verif threads

Benchmark #1: ./parities/parity-ao-fast-trie --mode=offline --scale-verifiers --num-verifiers=8 --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     288.779 s ±  8.539 s    [User: 274.010 s, System: 41.654 s]
  Range (min … max):   279.360 s … 301.326 s    5 runs

Forked rocksdb + 10 verif threads (master, Nov 20)

Benchmark #1: ./parities/parity-master --mode=offline --scale-verifiers --num-verifiers=8 --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     311.093 s ±  5.957 s    [User: 323.145 s, System: 40.403 s]
  Range (min … max):   301.396 s … 316.075 s    5 runs

Forked rocksdb + fast trie + 8 verif threads (master, Nov 21)

Benchmark #1: ./parities/parity-master-fast-trie --mode=offline --chain=eth --db-path=./current/bench-mainnet --pruning-history=12000 import ./blocks/blocks-5000.bin
  Time (mean ± σ):     322.392 s ±  7.124 s    [User: 341.089 s, System: 40.680 s]
  Range (min … max):   314.879 s … 333.632 s    5 runs

@cheme
Copy link
Contributor

cheme commented Nov 21, 2019

That's very interesting, trie db crate improvement are mainly on triedb iterator,
it is possible that it does not matter a lot for ethereum.
I suspect the loss come from decoding where we first build a nodeplan and then call 'build' function to go back to 'Node'. Before the pr 'Node' was directly produce from 'decode'. IIrc perf degrades on trie iter the same way at first but @jimpo fix that by avoiding 'build' calls it gets better. Probably it is the same here: we should avoid calling 'build' in triedbmut by having 'Node' replaced by some struct like '(data_slice, NodePlan)' but it is just a wild guess.

It could be rather simple to test: just put back the old 'decode' function in node_codec and see if it fix the perf (decode still exist in the trait but with a default implementation that call the 'build' function).

@grbIzl
Copy link
Collaborator

grbIzl commented Nov 22, 2019

@ordian @dvdplm thanks for info! Looks promising. May be worth mentioning in PR's description also.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Nov 25, 2019

It could be rather simple to test: just put back the old 'decode' function in node_codec and see if it fix the perf (decode still exist in the trait but with a default implementation that call the 'build' function).

I did this and but my micro-benchmark did not show any meaningful regression, not sure if my benchmark is bad perhaps. :/

* master:
  [ethcore]: apply filter when `PendingSet::AlwaysQueue` in `ready_transactions_filtered` (#11227)
  Update lib.rs (#11286)
  Don't prune ancient state when instantiating a Client (#11270)
  fixed verify_uncles error type (#11276)
  ethcore: fix rlp deprecation warnings (#11280)
@cheme
Copy link
Contributor

cheme commented Nov 25, 2019

I did this and but my micro-benchmark did not show any meaningful regression, not sure if my benchmark is bad perhaps. :/

Thanks for checking 👍 , should be from somewhere else then :\ .

@General-Beck
Copy link
Contributor

I think it will be necessary to test the build on the architectures aarch64, armv7, i386. And also on Windows and Mac platforms.
parity-clib library for android needs a separate test on the android device.

@General-Beck
Copy link
Contributor

build test's https://gitlab.parity.io/General-Beck/parity-ethereum/pipelines/70139
CI updated, on Linux hosts installed clang-8, on windows clang-9

@dvdplm
Copy link
Collaborator Author

dvdplm commented Nov 26, 2019

Linux hosts installed clang-8, on windows clang-9

Why not the same version on both? i.e. clang-9?

@General-Beck
Copy link
Contributor

General-Beck commented Nov 26, 2019

Why not the same version on both? i.e. clang-9?

During the discussion phase, I and @TriplEight do not mind using stable clang-9. We only need confirmation from the developers. and so that it does not interfere with other projects

@dvdplm
Copy link
Collaborator Author

dvdplm commented Nov 26, 2019

confirmation from the developers. and so that it does not interfere with other projects

Hmm, how do you mean, "developers"? And I'm not sure which other projects share the same CI setup, can you elaborate?

@ordian ordian marked this pull request as ready for review November 28, 2019 15:17
@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. F5-task 🎬 Generic label for things that just have to get done but are no issues in first place. labels Nov 28, 2019
@ordian ordian added this to the 2.7 milestone Nov 28, 2019
@dvdplm
Copy link
Collaborator Author

dvdplm commented Dec 1, 2019

Let me clarify. We've decided to move all our CI environments to clang-9.

Please let us know when the work is completed, thanks!

@General-Beck
Copy link
Contributor

build passed

@ordian ordian requested review from niklasad1 and tomusdrw and removed request for niklasad1 December 3, 2019 11:27
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Great! Do we have some benchmarks? NVM just saw in the comments.

parity/db/rocksdb/helpers.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice, LGTM

* master:
  add support for evan.network chains (#11289)
  Add benchmarks and tests for RlpNodeCodec decoding (#11287)
  upgrade vergen to 3.0 (#11293)
  interruptible test and build jobs (#11294)
  Istanbul HF on POA Sokol (#11282)
@ordian
Copy link
Collaborator

ordian commented Dec 3, 2019

Do we have some benchmarks?

I've talked to the devops team about running the import benches on a daily basis or at least on-demand to track perf regressions, will let you know once it's done.

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 3, 2019
@ordian ordian merged commit f6c3d4c into master Dec 3, 2019
@ordian ordian deleted the dp/chore/use-upstream-rocksdb branch December 3, 2019 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants