cometbft / cometbft-db

Database wrapper for CometBFT
Apache License 2.0
29 stars 51 forks source link

feat: add pebbledb #112

Closed faddat closed 9 months ago

faddat commented 10 months ago

This is a basic PR adding pebbledb.

Its license remains unchanged, apache 2.

Closes https://github.com/cometbft/cometbft-db/issues/90

codecov[bot] commented 10 months ago

Codecov Report

Attention: 69 lines in your changes are missing coverage. Please review.

Comparison is base (f7a229f) 77.53% compared to head (4616932) 77.15%.

:exclamation: Current head 4616932 differs from pull request most recent head d74f5de. Consider uploading reports for the commit d74f5de to get more accurate results

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112/graphs/tree.svg?width=650&height=150&src=pr&token=QcxCp5s2sq&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft)](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft) ```diff @@ Coverage Diff @@ ## main #112 +/- ## ========================================== - Coverage 77.53% 77.15% -0.39% ========================================== Files 22 23 +1 Lines 1785 2057 +272 ========================================== + Hits 1384 1587 +203 - Misses 342 399 +57 - Partials 59 71 +12 ``` | [Files](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft) | Coverage Δ | | |---|---|---| | [db.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-ZGIuZ28=) | `35.00% <ø> (ø)` | | | [pebble.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-cGViYmxlLmdv) | `74.63% <74.63%> (ø)` | | | [Files](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft) | Coverage Δ | | |---|---|---| | [db.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-ZGIuZ28=) | `35.00% <ø> (ø)` | | | [pebble.go](https://app.codecov.io/gh/cometbft/cometbft-db/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cometbft#diff-cGViYmxlLmdv) | `74.63% <74.63%> (ø)` | |
faddat commented 10 months ago

Here's why:

goos: darwin
goarch: arm64
pkg: github.com/cometbft/cometbft-db
BenchmarkGoLevelDBRandomReadsWrites-12        341013          3626 ns/op
BenchmarkMemDBRangeScans1M-12                   1035       1120334 ns/op
BenchmarkMemDBRangeScans10M-12                  1024       1153464 ns/op
BenchmarkMemDBRandomReadsWrites-12           1000000          1900 ns/op
BenchmarkDB/Keys_1000e3_Values_1024_pebbledb-12             1000000000           0.01967 ns/op
BenchmarkDB/Keys_1000e3_Values_1024_goleveldb-12            1000000000           0.008117 ns/op
BenchmarkDB/Keys_1000e3_Values_2048_pebbledb-12             1000000000           0.02024 ns/op
BenchmarkDB/Keys_1000e3_Values_2048_goleveldb-12            1000000000           0.01085 ns/op
BenchmarkDB/Keys_1000e3_Values_4096_pebbledb-12             1000000000           0.02399 ns/op
BenchmarkDB/Keys_1000e3_Values_4096_goleveldb-12            1000000000           0.02797 ns/op
BenchmarkDB/Keys_1000e3_Values_8192_pebbledb-12             1000000000           0.02593 ns/op
BenchmarkDB/Keys_1000e3_Values_8192_goleveldb-12            1000000000           0.04745 ns/op
BenchmarkDB/Keys_1000e3_Values_16384_pebbledb-12            1000000000           0.02701 ns/op
BenchmarkDB/Keys_1000e3_Values_16384_goleveldb-12           1000000000           0.1009 ns/op
BenchmarkDB/Keys_1000e3_Values_32768_pebbledb-12            1000000000           0.03281 ns/op
BenchmarkDB/Keys_1000e3_Values_32768_goleveldb-12           1000000000           0.1990 ns/op
BenchmarkDB/Keys_10000e3_Values_1024_pebbledb-12            1000000000           0.06059 ns/op
BenchmarkDB/Keys_10000e3_Values_1024_goleveldb-12           1000000000           0.1255 ns/op
BenchmarkDB/Keys_10000e3_Values_2048_pebbledb-12            1000000000           0.06427 ns/op
BenchmarkDB/Keys_10000e3_Values_2048_goleveldb-12           1000000000           0.1891 ns/op
BenchmarkDB/Keys_10000e3_Values_4096_pebbledb-12            1000000000           0.07321 ns/op
BenchmarkDB/Keys_10000e3_Values_4096_goleveldb-12           1000000000           0.3783 ns/op
BenchmarkDB/Keys_10000e3_Values_8192_pebbledb-12            1000000000           0.09354 ns/op
BenchmarkDB/Keys_10000e3_Values_8192_goleveldb-12           
1000000000           0.7913 ns/op
BenchmarkDB/Keys_10000e3_Values_16384_pebbledb-12           1000000000           0.1290 ns/op
BenchmarkDB/Keys_10000e3_Values_16384_goleveldb-12                 1    1625503416 ns/op
BenchmarkDB/Keys_10000e3_Values_32768_pebbledb-12           1000000000           0.1941 ns/op
BenchmarkDB/Keys_10000e3_Values_32768_goleveldb-12                 1    4447018291 ns/op
faddat commented 9 months ago

In order to write good benchmarks we need to merge this.

faddat commented 9 months ago

FYI, this one JUST AND ONLY adds pebble.

jmalicevic commented 9 months ago

So it seems that golevelDB is only more performant when we have small keys and write fewer of them (the first few lines). I am wondering what would the numbers look like for 64KB requests (which is the default block part size in Comet) , so it would be great to benchmark that. In any case, lets work on merging this PR so there is support for PebbleDB.

faddat commented 9 months ago

I think that #115 is overall better and is better for review. This PR is a cherry pick of @baabeetaa 's 2 year old work, and is missing things like the stats method, which are in #115

115 also uses a more recent pebbledb.

adizere commented 9 months ago

Should we just keep #115 and close this then? Or is there anything specific in this PR that is valuable?

jmalicevic commented 9 months ago

Should we just keep #115 and close this then? Or is there anything specific in this PR that is valuable?

I don't have any objections, the reason I went over this one is that it was clean and only changing Pebble. I would like to avoid changing existing behaviour of pre-existing DBs in #115 but otherwise no objections to closing this in favor of #115.

baabeetaa commented 9 months ago

I run iavl bench Large for:

the differences are several percentages.

So could set ForceSync=1 as default to avoid the upgrading issue without hurting performance much.

baabeetaa commented 9 months ago

There is another issue is that batch size max 4GB only. Afaik, only the recent terra classic upgrade had this issue with pebbledb.

faddat commented 9 months ago

@jmalicevic I think @baabeetaa made this one the PR you were looking for :).

faddat commented 9 months ago

@adizere @jmalicevic -

@baabeetaa let me know today that this PR is his preferred starting point and indeed, this is the atomic PR that only adds pebble :).

melekes commented 9 months ago

@baabeetaa or @faddat - could you please resolve conflicts?

melekes commented 9 months ago

This PR also lacks a changelog entry. Please add one ☝️

melekes commented 9 months ago

Changelog entry must be added in .changelog (we use unclog to manage our changelog)