feat: add Pebble DB backend #1069

wolkykim · 2023-08-25T09:53:47Z

add Pebble DB backend for experiments
related to Refactor DB Backends #1013

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

Benchmark output, based on the random read/write test patterns we have, shows faster op time compared to the default LevelDB backend. Note that this is Async read/write)

$ go test -cpu 1,4,10 -benchmem -run=^$ -bench=. -tags pebbledb
goos: darwin
goarch: arm64
pkg: github.com/gnolang/gno/tm2/pkg/db
BenchmarkGoLevelDBRandomReadsWrites       	  313208	      4180 ns/op	     672 B/op	      13 allocs/op
BenchmarkGoLevelDBRandomReadsWrites-4     	  307611	      4138 ns/op	     680 B/op	      13 allocs/op
BenchmarkGoLevelDBRandomReadsWrites-10    	  316270	      4081 ns/op	     679 B/op	      13 allocs/op

BenchmarkPebbleDBRandomReadsWrites          	  359060	      3742 ns/op	      41 B/op	       3 allocs/op
BenchmarkPebbleDBRandomReadsWrites-4        	  376674	      3335 ns/op	      32 B/op	       3 allocs/op
BenchmarkPebbleDBRandomReadsWrites-10       	  390951	      3384 ns/op	      32 B/op	       3 allocs/op

## When PebbleDB is tuned to use 16MB cache (default 8MB)
BenchmarkPebbleDBRandomReadsWrites          	  347396	      3470 ns/op	      42 B/op	       3 allocs/op
BenchmarkPebbleDBRandomReadsWrites-4        	  368671	      3011 ns/op	      31 B/op	       3 allocs/op
BenchmarkPebbleDBRandomReadsWrites-10       	  346952	      3000 ns/op	      32 B/op	       3 allocs/op

$ go test . -tags pebbledb
ok  	github.com/gnolang/gno/tm2/pkg/db	2.151s

$ go test . -tags pebbledb -v | grep -i pebbledb
=== RUN   TestBackendsGetSetDelete/pebbledb
    --- PASS: TestBackendsGetSetDelete/pebbledb (0.06s)
=== RUN   TestBackendsNilKeys/Testing_pebbledb
    --- PASS: TestBackendsNilKeys/Testing_pebbledb (0.08s)
=== RUN   TestDBIterator/pebbledb
    --- PASS: TestDBIterator/pebbledb (0.03s)
=== RUN   TestDBIteratorSingleKey/Backend_pebbledb
    --- PASS: TestDBIteratorSingleKey/Backend_pebbledb (0.04s)
=== RUN   TestDBIteratorTwoKeys/Backend_pebbledb
    --- PASS: TestDBIteratorTwoKeys/Backend_pebbledb (0.05s)
=== RUN   TestDBIteratorMany/Backend_pebbledb
    --- PASS: TestDBIteratorMany/Backend_pebbledb (0.03s)
=== RUN   TestDBIteratorEmpty/Backend_pebbledb
    --- PASS: TestDBIteratorEmpty/Backend_pebbledb (0.04s)
=== RUN   TestDBIteratorEmptyBeginAfter/Backend_pebbledb
    --- PASS: TestDBIteratorEmptyBeginAfter/Backend_pebbledb (0.04s)
=== RUN   TestDBIteratorNonemptyBeginAfter/Backend_pebbledb
    --- PASS: TestDBIteratorNonemptyBeginAfter/Backend_pebbledb (0.04s)
=== RUN   TestPebbleDB
--- PASS: TestPebbleDB (0.04s)
=== RUN   TestPrefixIteratorNoMatchNil/Prefix_w/_backend_pebbledb
    --- PASS: TestPrefixIteratorNoMatchNil/Prefix_w/_backend_pebbledb (0.04s)
=== RUN   TestPrefixIteratorNoMatch1/Prefix_w/_backend_pebbledb
    --- PASS: TestPrefixIteratorNoMatch1/Prefix_w/_backend_pebbledb (0.05s)
=== RUN   TestPrefixIteratorNoMatch2/Prefix_w/_backend_pebbledb
    --- PASS: TestPrefixIteratorNoMatch2/Prefix_w/_backend_pebbledb (0.04s)
=== RUN   TestPrefixIteratorMatch1/Prefix_w/_backend_pebbledb
    --- PASS: TestPrefixIteratorMatch1/Prefix_w/_backend_pebbledb (0.05s)
=== RUN   TestPrefixIteratorMatches1N/Prefix_w/_backend_pebbledb
    --- PASS: TestPrefixIteratorMatches1N/Prefix_w/_backend_pebbledb (0.06s)

moul · 2023-09-05T16:07:11Z

LGTM, could you rebase master and go mod tidy, please?

Edit: did it myself, now just looking for a second review.

This reverts commit c007c2c.

Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

ajnavarro · 2023-09-06T08:36:15Z

tm2/pkg/db/pebble_db.go

+	if err != nil {
+		if errors.Is(err, pebble.ErrNotFound) {
+			return nil
+		}
+		panic(err)
+	}


Avoid unnecessary nesting:

Suggested change

if err != nil {

if errors.Is(err, pebble.ErrNotFound) {

return nil

}

panic(err)

}

if errors.Is(err, pebble.ErrNotFound) {

return nil

}

if err != nil {

panic(err)

}

I think for most of the cases where err has nil value, the suggested logic will cost more as it goes through both L57 and L60. I honestly don't see why the suggestion is a better way.

I honestly don't see why the suggestion is a better way.

It is easier to read. If the code path is so hot that an extra if check will affect performance meaningfully, should we add some benchmarks to be careful about that?

Certainly, prioritizing readability and simplicity is beneficial, even in smaller cases. By consistently emphasizing clarity, we foster good habits and ensure our work remains accessible to others. It's a commendable practice to always strike a balance between efficiency and readability.

See #1111

ajnavarro · 2023-09-06T08:36:48Z

tm2/pkg/db/pebble_db.go

+	if err != nil {
+		if errors.Is(err, pebble.ErrNotFound) {
+			return false
+		}
+		panic(err)
+	}


Same as previous comment here

ajnavarro · 2023-09-06T08:39:59Z

tm2/pkg/db/pebble_db.go

+
+// Implements Batch.
+func (ba *pebbleDBBatch) Set(key, value []byte) {
+	ba.batch.Set(nonNilBytes(key), nonNilBytes(value), nil)


unhandled error.

I've added an error check on this.

ajnavarro · 2023-09-06T08:40:03Z

tm2/pkg/db/pebble_db.go

+
+// Implements Batch.
+func (ba *pebbleDBBatch) Delete(key []byte) {
+	ba.batch.Delete(nonNilBytes(key), nil)


unhandled error.

also addressed

wolkykim · 2023-09-06T13:29:29Z

@moul just so you know, I see you updated the go.mod file and now it builds against the latest of the pebbled's master branch. Did you intend to do so? Previously I pinned the version to use the latest crl-release-23.1 version. Anyway, there are small breaking changes in their iterator's return and I've updated the code accordingly. I think for experimental purposes it's fine not to pin it down, FYI.

wolkykim requested review from jaekwon, moul and a team as code owners August 25, 2023 09:53

github-actions bot assigned wolkykim Aug 25, 2023

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Aug 25, 2023

wolkykim closed this Sep 2, 2023

wolkykim reopened this Sep 2, 2023

moul approved these changes Sep 5, 2023

View reviewed changes

wolkykim and others added 10 commits September 5, 2023 18:08

feat: add Pebble DB backend

163d24d

test: add Pebble DB benchmark tests

a44fbfb

build: update go.mod

23c0771

Revert "build: update go.mod"

cb58157

This reverts commit c007c2c.

style: variable naming

d24765a

use default tunes

fb9e5da

style: rename isReverse

4f5ad34

style

a3bc628

style

0dfa54f

style: renmae test name

13503f0

Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

ajnavarro reviewed Sep 6, 2023

View reviewed changes

add err checks

67a42ab

moul added this to the 💡Someday/Maybe milestone Sep 6, 2023

wolkykim closed this Sep 12, 2023

wolkykim deleted the pebbledb branch September 12, 2023 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Pebble DB backend #1069

feat: add Pebble DB backend #1069

wolkykim commented Aug 25, 2023 •

edited

Loading

moul commented Sep 5, 2023 •

edited

Loading

ajnavarro Sep 6, 2023

wolkykim Sep 6, 2023 •

edited

Loading

ajnavarro Sep 11, 2023

moul Sep 11, 2023

ajnavarro Sep 6, 2023

ajnavarro Sep 6, 2023

wolkykim Sep 6, 2023

ajnavarro Sep 6, 2023

wolkykim Sep 6, 2023

wolkykim commented Sep 6, 2023 •

edited

Loading

feat: add Pebble DB backend #1069

feat: add Pebble DB backend #1069

Conversation

wolkykim commented Aug 25, 2023 • edited Loading

moul commented Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

wolkykim Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolkykim commented Sep 6, 2023 • edited Loading

wolkykim commented Aug 25, 2023 •

edited

Loading

moul commented Sep 5, 2023 •

edited

Loading

wolkykim Sep 6, 2023 •

edited

Loading

wolkykim commented Sep 6, 2023 •

edited

Loading