Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(v2): reduce leaf I/O #1044

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 160 additions & 0 deletions v2/cmd/bench/bench.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
package bench

import (
"log/slog"
"net/http"
"os"
"runtime/pprof"
"time"

"github.com/cosmos/iavl/v2"
"github.com/cosmos/iavl/v2/metrics"
"github.com/cosmos/iavl/v2/testutil"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
"github.com/prometheus/client_golang/prometheus/promhttp"
"github.com/spf13/cobra"
)

func Command() *cobra.Command {
cmd := &cobra.Command{
Use: "bench",
Short: "run benchmarks",
}
cmd.AddCommand(benchCommand())
return cmd
}

func benchCommand() *cobra.Command {
var (
dbPath string
changelogPath string
loadSnapshot bool
usePrometheus bool
cpuProfile string
)
cmd := &cobra.Command{
Use: "std",
Short: "run the std development benchmark",
Long: `Runs a longer benchmark for the IAVL tree. This is useful for development and testing.
Pre-requisites this command:
$ go run ./cmd gen tree --db /tmp/iavl-v2 --limit 1 --type osmo-like-many
mkdir -p /tmp/osmo-like-many/v2 && go run ./cmd gen emit --start 2 --limit 1000 --type osmo-like-many --out /tmp/osmo-like-many/v2

Optional for --snapshot arg:
$ go run ./cmd snapshot --db /tmp/iavl-v2 --version 1
`,

RunE: func(_ *cobra.Command, _ []string) error {
if cpuProfile != "" {
f, err := os.Create(cpuProfile)
if err != nil {
return err
}
if err := pprof.StartCPUProfile(f); err != nil {
return err
}
defer func() {
pprof.StopCPUProfile()
f.Close()
}()
}
treeOpts := iavl.DefaultTreeOptions()
treeOpts.CheckpointInterval = 80
treeOpts.StateStorage = true
treeOpts.HeightFilter = 1
treeOpts.EvictionDepth = 22
treeOpts.MetricsProxy = metrics.NewStructMetrics()
if usePrometheus {
treeOpts.MetricsProxy = newPrometheusMetricsProxy()
}

logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{AddSource: true}))
var multiTree *iavl.MultiTree
if loadSnapshot {
var err error
multiTree, err = iavl.ImportMultiTree(logger, 1, dbPath, treeOpts)
if err != nil {
return err
}
} else {
multiTree = iavl.NewMultiTree(logger, dbPath, treeOpts)
if err := multiTree.MountTrees(); err != nil {
return err
}
if err := multiTree.LoadVersion(1); err != nil {
return err
}
if err := multiTree.WarmLeaves(); err != nil {
return err
}
}

opts := testutil.CompactedChangelogs(changelogPath)
opts.SampleRate = 250_000

// opts.Until = 1_000
// opts.UntilHash = "557663181d9ab97882ecfc6538e3b4cfe31cd805222fae905c4b4f4403ca5cda"
opts.Until = 500
opts.UntilHash = "2670bd5767e70f2bf9e4f723b5f205759e39afdb5d8cfb6b54a4a3ecc27a1377"

_, err := multiTree.TestBuild(opts)
return err
},
}
cmd.Flags().StringVar(&dbPath, "db", "/tmp/iavl-v2", "the path to the database at version 1")
cmd.Flags().StringVar(&changelogPath, "changelog", "/tmp/osmo-like-many/v2", "the path to the changelog")
cmd.Flags().BoolVar(&loadSnapshot, "snapshot", false, "load the snapshot at version 1 before running the benchmarks (loads full tree into memory)")
cmd.Flags().BoolVar(&usePrometheus, "prometheus", false, "enable prometheus metrics")
cmd.Flags().StringVar(&cpuProfile, "cpu-profile", "", "write cpu profile to file")

if err := cmd.MarkFlagRequired("changelog"); err != nil {
panic(err)
}
if err := cmd.MarkFlagRequired("db"); err != nil {
panic(err)
}
Comment on lines +111 to +116
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace panic with proper error handling in flag validation.

Using panic for flag validation is not a good practice. Since this is happening during command initialization, return an error instead.

-	if err := cmd.MarkFlagRequired("changelog"); err != nil {
-		panic(err)
-	}
-	if err := cmd.MarkFlagRequired("db"); err != nil {
-		panic(err)
-	}
+	for _, flag := range []string{"changelog", "db"} {
+		if err := cmd.MarkFlagRequired(flag); err != nil {
+			return nil
+		}
+	}

Committable suggestion skipped: line range outside the PR's diff.

return cmd
}

var _ metrics.Proxy = &prometheusMetricsProxy{}

type prometheusMetricsProxy struct {
workingSize prometheus.Gauge
workingBytes prometheus.Gauge
}

func newPrometheusMetricsProxy() *prometheusMetricsProxy {
p := &prometheusMetricsProxy{}
p.workingSize = promauto.NewGauge(prometheus.GaugeOpts{
Name: "iavl_working_size",
Help: "working size",
})
p.workingBytes = promauto.NewGauge(prometheus.GaugeOpts{
Name: "iavl_working_bytes",
Help: "working bytes",
})
http.Handle("/metrics", promhttp.Handler())
go func() {
err := http.ListenAndServe(":2112", nil)
if err != nil {
panic(err)
}
}()
Comment on lines +137 to +143
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle HTTP server errors more gracefully.

The current implementation panics on HTTP server errors, which is not ideal for a long-running service.

-	go func() {
-		err := http.ListenAndServe(":2112", nil)
-		if err != nil {
-			panic(err)
-		}
-	}()
+	go func() {
+		if err := http.ListenAndServe(":2112", nil); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			logger.Error("metrics server failed", "error", err)
+		}
+	}()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
http.Handle("/metrics", promhttp.Handler())
go func() {
err := http.ListenAndServe(":2112", nil)
if err != nil {
panic(err)
}
}()
http.Handle("/metrics", promhttp.Handler())
go func() {
if err := http.ListenAndServe(":2112", nil); err != nil && !errors.Is(err, http.ErrServerClosed) {
logger.Error("metrics server failed", "error", err)
}
}()

return p
}

func (p *prometheusMetricsProxy) IncrCounter(_ float32, _ ...string) {
}

func (p *prometheusMetricsProxy) SetGauge(val float32, keys ...string) {
k := keys[1]
switch k {
case "working_size":
p.workingSize.Set(float64(val))
case "working_bytes":
p.workingBytes.Set(float64(val))
}
}
Comment on lines +150 to +158
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Make SetGauge more robust.

The current implementation assumes the keys slice has at least 2 elements and doesn't validate the key value.

 func (p *prometheusMetricsProxy) SetGauge(val float32, keys ...string) {
+	if len(keys) < 2 {
+		return
+	}
 	k := keys[1]
 	switch k {
 	case "working_size":
 		p.workingSize.Set(float64(val))
 	case "working_bytes":
 		p.workingBytes.Set(float64(val))
+	default:
+		// Log unexpected key for debugging
+		logger.Debug("unexpected metric key", "key", k)
 	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (p *prometheusMetricsProxy) SetGauge(val float32, keys ...string) {
k := keys[1]
switch k {
case "working_size":
p.workingSize.Set(float64(val))
case "working_bytes":
p.workingBytes.Set(float64(val))
}
}
func (p *prometheusMetricsProxy) SetGauge(val float32, keys ...string) {
if len(keys) < 2 {
return
}
k := keys[1]
switch k {
case "working_size":
p.workingSize.Set(float64(val))
case "working_bytes":
p.workingBytes.Set(float64(val))
default:
// Log unexpected key for debugging
logger.Debug("unexpected metric key", "key", k)
}
}


func (p *prometheusMetricsProxy) MeasureSince(_ time.Time, _ ...string) {}
4 changes: 2 additions & 2 deletions v2/cmd/gen/gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ func treeCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "tree",
Short: "build and save a Tree to disk, taking generated changesets as input",
RunE: func(cmd *cobra.Command, args []string) error {
multiTree := iavl.NewMultiTree(iavl.NewTestLogger(), dbPath, iavl.TreeOptions{StateStorage: true})
RunE: func(_ *cobra.Command, _ []string) error {
multiTree := iavl.NewMultiTree(iavl.NewDebugLogger(), dbPath, iavl.DefaultTreeOptions())
defer func(mt *iavl.MultiTree) {
err := mt.Close()
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions v2/cmd/root.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package main

import (
"github.com/cosmos/iavl/v2/cmd/bench"
"github.com/cosmos/iavl/v2/cmd/gen"
"github.com/cosmos/iavl/v2/cmd/rollback"
"github.com/cosmos/iavl/v2/cmd/scan"
Expand All @@ -19,6 +20,7 @@ func RootCommand() (*cobra.Command, error) {
rollback.Command(),
scan.Command(),
latestCommand(),
bench.Command(),
)
return cmd, nil
}
9 changes: 5 additions & 4 deletions v2/iterator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ func Test_Iterator(t *testing.T) {
sql, err := iavl.NewInMemorySqliteDb(pool)
require.NoError(t, err)

tree := iavl.NewTree(sql, pool, iavl.TreeOptions{StateStorage: false})
opts := iavl.DefaultTreeOptions()
tree := iavl.NewTree(sql, pool, opts)
set := func(key string, value string) {
_, err := tree.Set([]byte(key), []byte(value))
require.NoError(t, err)
Expand Down Expand Up @@ -226,7 +227,8 @@ func Test_IteratorTree(t *testing.T) {
sql, err := iavl.NewSqliteDb(pool, iavl.SqliteDbOptions{Path: tmpDir})
require.NoError(t, err)

tree := iavl.NewTree(sql, pool, iavl.TreeOptions{StateStorage: true})
opts := iavl.DefaultTreeOptions()
tree := iavl.NewTree(sql, pool, opts)
set := func(key string, value string) {
_, err := tree.Set([]byte(key), []byte(value))
require.NoError(t, err)
Expand All @@ -241,7 +243,7 @@ func Test_IteratorTree(t *testing.T) {

_, version, err := tree.SaveVersion()
require.NoError(t, err)
tree = iavl.NewTree(sql, pool, iavl.TreeOptions{StateStorage: true})
tree = iavl.NewTree(sql, pool, opts)
require.NoError(t, tree.LoadVersion(version))
cases := []struct {
name string
Expand Down Expand Up @@ -304,5 +306,4 @@ func Test_IteratorTree(t *testing.T) {
require.NoError(t, itr.Close())
})
}

}
9 changes: 8 additions & 1 deletion v2/logger.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package iavl

import "log/slog"
import (
"log/slog"
"os"
)

// Logger defines basic logger that IAVL expects.
// It is a subset of the cosmossdk.io/core/log.Logger interface.
Expand Down Expand Up @@ -39,6 +42,10 @@ func NewTestLogger() Logger {
return &testLogger{}
}

func NewDebugLogger() Logger {
return slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
}

type testLogger struct{}

func (l *testLogger) Info(msg string, keys ...any) {
Expand Down
Loading
Loading