feat: add pebbledb by faddat · Pull Request #112 · cometbft/cometbft-db

faddat · 2024-01-07T09:25:58Z

This is a basic PR adding pebbledb.

Its license remains unchanged, apache 2.

Closes #90

codecov · 2024-01-07T09:48:06Z

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%.

❗ 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

@@            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	Coverage Δ
db.go	`35.00% <ø> (ø)`
pebble.go	`74.63% <74.63%> (ø)`

Files	Coverage Δ
db.go	`35.00% <ø> (ø)`
pebble.go	`74.63% <74.63%> (ø)`

faddat · 2024-01-09T21:03:54Z

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 · 2024-01-14T18:54:53Z

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

faddat · 2024-01-15T21:05:12Z

FYI, this one JUST AND ONLY adds pebble.

make pebble the clear choice #115 tries to make it as fast as possible then benchmark in realworldish situations.

jmalicevic · 2024-01-19T10:24:32Z

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.

pebble.go

pebble_test.go

pebble.go

jmalicevic · 2024-01-19T10:39:39Z

pebble.go

+This is set at compile time. Could be 0 or 1, defaults is 0.
+It will force using Sync for NoSync functions (Set, Delete, Write)
+
+Used as a workaround for chain-upgrade issue: At the upgrade-block, the sdk will panic without flushing data to disk or


How was this handled with existing databases where we did not introduce this ForceSync flag?

I think that it may make more sense to go through #115 -- it uses pebbledb 1.0 and doesn't seem to need this, but @baabeetaa can provide more context I think.

i havent tried newer version of pebbledb. With current version, set it default to 1 instead.

btw, i'll test upgrading without ForceSync=1 with new version of pebbledb tomorrow and report.

Nice -- could you test the #115 branch?

ok, there will be Omniflix mainnet upgrade tomorrow.

i tried upgrading Omniflix with pebble v1.0.0. It doesnt work without using ForceSync

Imo, using default ForceSync=1 shoulnt be a problem.
https://github.com/cockroachdb/pebble/blob/master/options.go#L356

i updated default ForceSync=1, so no need to use this flag when upgrading.

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

faddat · 2024-01-21T21:39:24Z

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 · 2024-01-23T09:20:20Z

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

jmalicevic · 2024-01-23T09:30:51Z

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 · 2024-01-23T18:36:35Z

I run iavl bench Large for:

ForceSync=0 (default): https://hastebin.com/share/gixuwowecu.bash
ForceSync=1: https://hastebin.com/share/ogiqiligig.bash

the differences are several percentages.

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

melekes

Thanks @baabeetaa and @faddat ❤️

db.go

melekes · 2024-01-24T07:27:56Z

pebble.go

+
+Notice if ForceSync=0: performance will be better. However, there is an issue when upgrading.
+And the workaround (if using ForceSync=0):
+At the upgrade-block, the sdk will panic without flushing data to disk or closing dbs properly.


I didn't get it. Isn't the solution to use SetSync in the SDK? instead of forcing all ops to be sync

i agree. However, this isnt an issue with goleveldb which is the default db. And other dbs are experiments atm.

Does it not degrade performance if done needlessly?

Still, the right way is to patch Cosmos SDK, not leave this hack even though pebbledb is experimental. Please remove

Sir, it could be a big problem for node operators who run nodes for different chains and different sdk versions.
Also when syching archive nodes from genesis requires a lot of upgrades.
This hack is not the best way to fix. But it works.

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

melekes · 2024-01-24T12:24:38Z

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

melekes · 2024-01-24T12:25:19Z

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

melekes · 2024-01-24T13:34:23Z

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

CHANGELOG.md

.changelog/unreleased/feat/112.md

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

.changelog/unreleased/feat/112.md

renamed 112-add-pebbledb.md to 112-pebbledb.md

add pebbledb

6dc7c0c

faddat requested a review from a team as a code owner January 7, 2024 09:25

faddat added 2 commits January 7, 2024 16:34

use latest pebble

5c90ecd

Merge remote-tracking branch 'origin/main' into pebble

ef2dfbb

faddat mentioned this pull request Jan 7, 2024

chore: lint code #114

Merged

faddat closed this Jan 7, 2024

faddat reopened this Jan 9, 2024

This was referenced Jan 9, 2024

perf: enhance performance in state cometbft/cometbft#1977

Closed

Understand and simplify CometBFT database backend cometbft/cometbft#48

Closed

adizere linked an issue Jan 19, 2024 that may be closed by this pull request

Add support for pebbleDB #90

Closed

adizere added this to the 2024-Q1 milestone Jan 19, 2024

adizere assigned jmalicevic Jan 19, 2024

adizere removed this from the 2024-Q1 milestone Jan 19, 2024

jmalicevic reviewed Jan 19, 2024

View reviewed changes

faddat and others added 6 commits January 20, 2024 01:30

Update pebble.go

de4867b

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

Update pebble.go

d1257bb

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

Update pebble.go

99c3325

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

Update pebble.go

f3aae84

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

Update pebble.go

fdf1970

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

Update pebble.go

aaaa1c4

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>

faddat marked this pull request as draft January 21, 2024 21:38

melekes changed the title ~~add pebbledb (again)~~ feat: add pebbledb Jan 24, 2024

melekes reviewed Jan 24, 2024

View reviewed changes

melekes mentioned this pull request Jan 24, 2024

make pebble the clear choice #115

Closed

Update db.go

16835d1

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

melekes approved these changes Jan 24, 2024

View reviewed changes

melekes added this pull request to the merge queue Jan 24, 2024

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jan 24, 2024

baabeetaa added 3 commits January 24, 2024 20:01

resolve conflicts

a51503a

Merge branch 'main' into pebble

d551945

update CHANGELOG.md: Added support for pebbledb v1.0.0

c77abd2

faddat and others added 2 commits January 24, 2024 13:52

add unclog entry

06b6267

add changelog

e8e56f3

melekes reviewed Jan 24, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/pebble' into pebble

bcdd387

melekes reviewed Jan 24, 2024

View reviewed changes

.changelog/unreleased/feat/112.md Outdated Show resolved Hide resolved

faddat and others added 2 commits January 24, 2024 13:57

Update .changelog/unreleased/feat/112.md

4616932

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Merge remote-tracking branch 'origin/pebble' into pebble

ac168a6

melekes reviewed Jan 24, 2024

View reviewed changes

.changelog/unreleased/feat/112.md Outdated Show resolved Hide resolved

melekes reviewed Jan 24, 2024

View reviewed changes

.changelog/unreleased/feat/112.md Outdated Show resolved Hide resolved

baabeetaa added 2 commits January 24, 2024 21:02

add changelog...

567142e

add changelog...

d74f5de

renamed 112-add-pebbledb.md to 112-pebbledb.md

melekes enabled auto-merge January 24, 2024 14:42

melekes added this pull request to the merge queue Jan 24, 2024

Merged via the queue into cometbft:main with commit 2af169e Jan 24, 2024

melekes mentioned this pull request Jan 25, 2024

pebbledb: panic during upgrade w/ Cosmos SDK #126

Closed

gartnera mentioned this pull request Jul 23, 2024

chore: use cometbft-db zeta-chain/node#2536

Merged

Conversation

faddat commented Jan 7, 2024 • edited by melekes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

faddat commented Jan 9, 2024

Uh oh!

faddat commented Jan 14, 2024

Uh oh!

faddat commented Jan 15, 2024

Uh oh!

jmalicevic commented Jan 19, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

faddat commented Jan 21, 2024

Uh oh!

adizere commented Jan 23, 2024

Uh oh!

jmalicevic commented Jan 23, 2024

Uh oh!

baabeetaa commented Jan 23, 2024

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented Jan 24, 2024

Uh oh!

melekes commented Jan 24, 2024

Uh oh!

melekes commented Jan 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

faddat commented Jan 7, 2024 •

edited by melekes

Loading

codecov bot commented Jan 7, 2024 •

edited

Loading

melekes Jan 24, 2024 •

edited

Loading