cmd/evm: benchmarking via `statetest` command + filter by name, index and fork #30442

jwasinger · 2024-09-16T11:21:53Z

When evm statetest --bench is specified, benchmark the execution similarly to evm run. It will explicitly error if there is more than one matched subtest to be executed (new statetest flags are added to filter which tests are ran).

Breaks the state test runner API. But I figure that we are the only consumers of this API, so it's not a big problem?

…b all)

holiman

Checked to see if there was any noticeable slowdown.

Master:

$ time yes ~/go/src/github.com/holiman/goevmlab/examples/sstore_static/sstore_test.json | head -n 1000 | go run ./cmd/evm/ statetest 2>/dev/null 1>/dev/null 

real    0m3.389s
user    0m3.688s
sys     0m1.008s

This PR

$ time yes ~/go/src/github.com/holiman/goevmlab/examples/sstore_static/sstore_test.json | head -n 1000 | go run ./cmd/evm/ statetest 2>/dev/null 1>/dev/null 

real    0m3.409s
user    0m3.927s
sys     0m1.042s

So 👍

Just some minor nits, otherwise lgtm

cmd/evm/staterunner.go

holiman

LGTM

MariusVanDerWijden · 2024-09-25T06:10:06Z

cmd/evm/staterunner.go

+	} else if len(matchingTests) != 1 {
+		return fmt.Errorf("can only benchmark single state test case (more than one matching params)")
+	}
+	var gasUsed uint64
+	result := testing.Benchmark(func(b *testing.B) {
+		for i := 0; i < b.N; i++ {
+			test := matchingTests[0]
+			_, _, gasUsed, _ = test.test.RunNoVerify(test.st, cfg, false, rawdb.HashScheme)
+		}
+	})
+	var stats execStats
+	// Get the average execution time from the benchmarking result.
+	// There are other useful stats here that could be reported.
+	stats.time = time.Duration(result.NsPerOp())
+	stats.allocs = result.AllocsPerOp()
+	stats.bytesAllocated = result.AllocedBytesPerOp()
+	fmt.Fprintf(os.Stderr, `EVM gas used:    %d
+execution time:  %v
+allocations:     %d
+allocated bytes: %d
+`, gasUsed, stats.time, stats.allocs, stats.bytesAllocated)


I would move this into its own function and do something like this

if bench { runBenchmark() } return nil

which makes it a big cleaner imo

I think it's ok as is

MariusVanDerWijden

LGTM, one tiny nitpick that you could ignore if you want

cmd/evm/staterunner.go

holiman · 2024-09-30T14:40:44Z

cmd/evm/staterunner.go

+	} else if len(matchingTests) != 1 {
+		return fmt.Errorf("can only benchmark single state test case (more than one matching params)")
+	}
+	var gasUsed uint64
+	result := testing.Benchmark(func(b *testing.B) {
+		for i := 0; i < b.N; i++ {
+			test := matchingTests[0]
+			_, _, gasUsed, _ = test.test.RunNoVerify(test.st, cfg, false, rawdb.HashScheme)
+		}
+	})
+	var stats execStats
+	// Get the average execution time from the benchmarking result.
+	// There are other useful stats here that could be reported.
+	stats.time = time.Duration(result.NsPerOp())
+	stats.allocs = result.AllocsPerOp()
+	stats.bytesAllocated = result.AllocedBytesPerOp()
+	fmt.Fprintf(os.Stderr, `EVM gas used:    %d
+execution time:  %v
+allocations:     %d
+allocated bytes: %d
+`, gasUsed, stats.time, stats.allocs, stats.bytesAllocated)


I think it's ok as is

holiman · 2024-09-30T14:42:50Z

cmd/evm/staterunner.go

+	if !bench {
+		return nil
+	} else if len(matchingTests) != 1 {
+		return fmt.Errorf("can only benchmark single state test case (more than one matching params)")


Why though?
Wouldn't it make more sense to put the benchmarking into the loop above, and after each test.test.Run, you do a bench run, if so desired.
You might even consider integrating the bench stats into the results, but now I'm just thinking aloud... ?

Yes. Benchmark every test unless otherwise-specified sounds like fine default behavior to me.

SO drop this whole if-clause?

holiman · 2024-09-30T14:43:53Z

Sorry -- I know I approved it earlier, and I came around to merge it now, but then I found some more things that could be improved :)

jwasinger · 2024-10-01T08:22:07Z

Tried to re-run the exact test you posted from goevmlab: ~/go/src/github.com/holiman/goevmlab/examples/sstore_static/sstore_test.json but it appears that it isn't in the repo.

Running a separate test shows no measurable performance difference between the latest changes and master branch.

holiman · 2024-10-04T12:50:47Z

cmd/evm/runner.go

-	time           time.Duration // The execution time.
-	allocs         int64         // The number of heap allocations during execution.
-	bytesAllocated int64         // The cumulative number of bytes allocated during execution.
+type ExecStats struct {


Does this need to be exported? I don't see why

Ah I think I got confused because non-exported members of structs are not serialized in JSON so I assumed that the type also had to be exported 🤷‍♂️

Wait but how can StateTestResult have a public field with a private type...

holiman · 2024-10-04T12:51:40Z

cmd/evm/staterunner.go

+	if !bench {
+		return nil
+	} else if len(matchingTests) != 1 {
+		return fmt.Errorf("can only benchmark single state test case (more than one matching params)")


SO drop this whole if-clause?

cmd/evm/runner.go

holiman

LGTM

jwasinger added 3 commits September 16, 2024 16:40

state test benchmarking

74f41ac

add used gas

46040cc

some docs, rename helper func, specify default value for indices (glo…

96fbf66

…b all)

jwasinger requested review from holiman, MariusVanDerWijden and lightclient as code owners September 16, 2024 11:21

holiman requested changes Sep 24, 2024

View reviewed changes

cmd/evm/staterunner.go Outdated Show resolved Hide resolved

cmd/evm/staterunner.go Outdated Show resolved Hide resolved

cmd/evm/staterunner.go Show resolved Hide resolved

holiman changed the title ~~tests, cmd/evm: expand --bench flag support to include statetest subcommand. introduce test name, fork, index filtering flags for state test subcommand.~~ cmd/evm: benchmarking via statetest command + filter by name, index and fork Sep 24, 2024

jwasinger added 2 commits September 24, 2024 17:40

make requested changes

3060ae1

small nit

6b05f83

holiman reviewed Sep 24, 2024

View reviewed changes

cmd/evm/staterunner.go Outdated Show resolved Hide resolved

fix goimport

64cbd00

holiman approved these changes Sep 24, 2024

View reviewed changes

MariusVanDerWijden reviewed Sep 25, 2024

View reviewed changes

MariusVanDerWijden approved these changes Sep 25, 2024

View reviewed changes

holiman requested changes Sep 30, 2024

View reviewed changes

jwasinger added 4 commits September 30, 2024 22:02

unexport evm state test bench command flags

b8ed3de

incorporate ExecStats into StateTestResult. add gas used to ExecStats

20256a1

remove typo

7cf43bf

gofmt

cf77441

holiman requested changes Oct 4, 2024

View reviewed changes

jwasinger added 2 commits October 4, 2024 20:05

remove unused code

fedbdee

make ExecStats non-public

1f84917

holiman reviewed Nov 8, 2024

View reviewed changes

cmd/evm/runner.go Outdated Show resolved Hide resolved

holiman approved these changes Nov 8, 2024

View reviewed changes

holiman added this to the 1.14.12 milestone Nov 8, 2024

cmd/evm: make warm-up before bench-run

ca8beb9

holiman force-pushed the state-test-bench branch from 9f3c9fb to ca8beb9 Compare November 8, 2024 13:51

holiman merged commit d42d450 into ethereum:master Nov 8, 2024
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/evm: benchmarking via `statetest` command + filter by name, index and fork #30442

cmd/evm: benchmarking via `statetest` command + filter by name, index and fork #30442

jwasinger commented Sep 16, 2024

holiman left a comment

holiman left a comment

MariusVanDerWijden Sep 25, 2024

holiman Sep 30, 2024

MariusVanDerWijden left a comment

holiman Sep 30, 2024

holiman Sep 30, 2024

jwasinger Sep 30, 2024

holiman Oct 4, 2024

holiman commented Sep 30, 2024

jwasinger commented Oct 1, 2024

holiman Oct 4, 2024

jwasinger Oct 4, 2024

jwasinger Oct 4, 2024

holiman Oct 4, 2024

holiman left a comment

cmd/evm: benchmarking via statetest command + filter by name, index and fork #30442

cmd/evm: benchmarking via statetest command + filter by name, index and fork #30442

Conversation

jwasinger commented Sep 16, 2024

holiman left a comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariusVanDerWijden left a 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

holiman commented Sep 30, 2024

jwasinger commented Oct 1, 2024

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

holiman left a comment

Choose a reason for hiding this comment

cmd/evm: benchmarking via `statetest` command + filter by name, index and fork #30442

cmd/evm: benchmarking via `statetest` command + filter by name, index and fork #30442