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

cmd/evm: don't reuse state between iterations, show errors #30780

Merged
merged 12 commits into from
Nov 26, 2024

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Nov 21, 2024

Reusing state between benchmark iterations resulted in inconsistent results across runs, which surfaced in #30778 .

If these errors are triggered again, they should be printed to output. To ensure that the code invoking testing.Benchmark() can catch and output these errors, and then exit: I replace calls to t.B.Fatalf with setting an error which is consumed by the calling code.

The same consistency checks should probably be applied to the state test benchmarker. I am figuring that out now.

closes #30778 .

cmd/evm/runner.go Outdated Show resolved Hide resolved
cmd/evm/runner.go Outdated Show resolved Hide resolved
cmd/evm/runner.go Outdated Show resolved Hide resolved
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Some nits

cmd/evm/runner.go Outdated Show resolved Hide resolved
cmd/evm/runner.go Outdated Show resolved Hide resolved
cmd/evm/runner.go Outdated Show resolved Hide resolved
cmd/evm/runner.go Outdated Show resolved Hide resolved
@holiman holiman changed the title cmd/evm: don't reuse state between evm benchmark iterations. ensure benchmark errors are included in output. cmd/evm: don't reuse state between iterations, show errors Nov 22, 2024
jwasinger and others added 4 commits November 22, 2024 15:01
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.14.13 milestone Nov 25, 2024
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Even though you worked around the panic due to b.Fatalf, I think it would still be more "correct" to also invoke testing.Init() when you enter the if bench clause.

@holiman holiman merged commit 915248c into ethereum:master Nov 26, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

evm run --bench segfaults in Fatalf
2 participants