make pebble the clear choice #115
Conversation
|
All I can say is:
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
- Coverage 77.53% 73.98% -3.55%
==========================================
Files 22 13 -9
Lines 1785 1134 -651
==========================================
- Hits 1384 839 -545
+ Misses 342 250 -92
+ Partials 59 45 -14
... and 10 files with indirect coverage changes
|
|
faddat@Whites-MacBook-Pro cometbft-db % GOARCH=arm64 go test -bench=BenchmarkPebbleDBvsGoLevelDB |
|
faddat@Whites-MacBook-Pro cometbft-db % GOARCH=arm64 go test -bench=BenchmarkPebbleDBvsGoLevelDB |
|
However if tested against iavl, this would be even more superieor. |
|
Please use this mainly for the benchmarks as I think I made a kind of significant finding here with regard to high bandwidth situations. |
|
TO: Maintainer - please run the benchmarks I think they could explain some recent stuff. |
|
I'll pull benchmarks from here. |
|
to: maintiner (idk who this is) please run benchmarks on a solidly fast machine. I fear if we don't it wont match the real world. |
|
In general the benchmarks will exceed what a normal computer can do. my 64gb macbook pro handles it well. I could reduce the size of values, which I am somewhat guessing on, and increase the number of keys. The critical thing to note is that pebble is in general many times faster. Ulike goleveldb, pebble is maintained. |
|
sometimes pebble is a few million times faster. Flakiness, anyone? |
This reverts commit a574117.
|
the big worry here is that goleveldb does not act in a predictable manner. |
|
preparing a branch of cometbft's main to see if we can eliminate time based flakiness with pebble. |
In my view the berachain testnet experienced unreliability due to upstream issues in Comet. * cometbft/cometbft#1940 While I cannot be sure, I think that some of the results that I got in recent benchmarks of goleveldb actually represent clear bugs that could under certain circumstances be used to halt chains. * cometbft/cometbft-db#115 So here's the thinking: 1) drop goleveldb for pebble by literally specifying pebble statically in the code 2) reduce max_bytes to 1mb 3) increase the size of the validator set and distribute VP evenly 4) torture the network, including torturing RPCs Co-authored-by: Devon Bear <[email protected]>
|
I'm a fan. How is pebbleDB handling workload in Berachain? Really wish you didn't do unnecessary changes here so we could review it easier. But aside from that, concept ACK and we'll review this ASAP, thanks for patience! |
| @@ -12,7 +12,7 @@ import ( | |||
| "github.com/dgraph-io/badger/v2" | |||
| ) | |||
|
|
|||
| func init() { registerDBCreator(BadgerDBBackend, badgerDBCreator, true) } | |||
There was a problem hiding this comment.
Is there a specific reason for changing this interface?
| @@ -54,7 +58,6 @@ test-all-with-coverage: | |||
| -race \ | |||
| -coverprofile=coverage.txt \ | |||
| -covermode=atomic \ | |||
| -tags=memdb,goleveldb,cleveldb,boltdb,rocksdb,grocksdb_clean_link,badgerdb \ | |||
There was a problem hiding this comment.
Why are we removing this line?
| @@ -137,7 +137,7 @@ func (db *MemDB) DeleteSync(key []byte) error { | |||
| } | |||
|
|
|||
| // Close implements DB. | |||
| func (db *MemDB) Close() error { | |||
| func (*MemDB) Close() error { | |||
There was a problem hiding this comment.
I think lint will complain that the signature is not following the same pattern for all functions. It's probably a style preference so not a strong opinion but it usually complains in case of mixed (db *MemDB) and (*MemDB).
There was a problem hiding this comment.
Ah, nope, please think of an unused receiver as the declaration of an unused variable, and these lints will make perfect sense to you.
| "github.com/cockroachdb/pebble/bloom" | ||
| ) | ||
|
|
||
| // ForceSync |
There was a problem hiding this comment.
Have we established whether this version of Pebble needs these extra steps?
There was a problem hiding this comment.
It needs them, as long as SDK doesn't cleanly. close the DB after an upgrade -- at least I think that's what @baabeetaa found.
…erwhelmingly-fast
- prefix(github.com/cosmos/ibc-go)
|
Closing in favor of #112. Let's focus on a single PR. After it's merged, we can do linting. |
Closes: #90