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

test: add performance benchmarks for the phase0 block attributes #6145

Merged
merged 15 commits into from
Dec 5, 2023

Conversation

nazarhussain
Copy link
Contributor

@nazarhussain nazarhussain commented Nov 30, 2023

Motivation

Observe the performance regressions.

Description

Add a base performance benchmark for phase0 block attributes.

Added new metric and a graph, which will helps us to identify any issue with the individual processing steps.

image

part of #5793

Steps to test or reproduce

Run all tests.

@nazarhussain nazarhussain requested a review from a team as a code owner November 30, 2023 17:38
@nazarhussain nazarhussain self-assigned this Nov 30, 2023
@nazarhussain nazarhussain force-pushed the nh/5793-phase0-performance branch from cd9c65e to 68611d2 Compare December 4, 2023 09:58
@nazarhussain nazarhussain requested a review from twoeths December 5, 2023 08:50
executionBlockProductionTimeSteps: register.histogram<"step">({
name: "beacon_block_production_execution_steps_seconds",
help: "Detailed steps runtime of execution block production",
buckets: [0.1, 1, 2, 4, 10],
Copy link
Contributor

Choose a reason for hiding this comment

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

buckets value seems to big for each step. If a step takes more than 1s then it's already an issue so I suggest removing 2, 4, 10 and track more < 1 buckets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the buckets completely, as these are not intended in the metric graph we need.

@nazarhussain nazarhussain requested a review from twoeths December 5, 2023 10:42
@nazarhussain
Copy link
Contributor Author

As the failed perf tests are not related to the code changes in this PR, so why merging it.

@nazarhussain nazarhussain merged commit 2efd0e8 into unstable Dec 5, 2023
15 of 16 checks passed
@nazarhussain nazarhussain deleted the nh/5793-phase0-performance branch December 5, 2023 16:22
name: "beacon_block_production_execution_steps_seconds",
help: "Detailed steps runtime of execution block production",
buckets: [0.01, 0.1, 0.2, 0.5, 1],
/**
Copy link
Member

Choose a reason for hiding this comment

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

I feel like commenting those values here is not the right approach. We should use an enum in such cases as we did for block production source.

Might be worth to look into how we could make label values type safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to change the types from pom-client for that. That's a bigger task to do. Feel free to create a separate issue to track in future.

@nflaig
Copy link
Member

nflaig commented Dec 5, 2023

As the failed perf tests are not related to the code changes in this PR, so why merging it.

@nazarhussain are you sure about that? it failed with same errors again on unstable benchmark run https://github.com/ChainSafe/lodestar/actions/runs/7103589338/job/19336631592 while before it was consistently passing.

@nazarhussain
Copy link
Contributor Author

nazarhussain commented Dec 5, 2023

@nazarhussain are you sure about that? it failed with same errors again on unstable benchmark run

Some of 'state-transition' perf tests were failing earlier. And this PR didn't changed any code for that package.

Will look on unstable branch tests in some time.

@dapplion
Copy link
Contributor

dapplion commented Dec 6, 2023

As the failed perf tests are not related to the code changes in this PR, so why merging it.

Are you sure? Please investigate

jeluard pushed a commit that referenced this pull request Dec 7, 2023
* Add opPool benchmark

* Add benchmark for empty block body

* Optimize a skipping condition

* Fix the identifier for a graph

* Add detailed steps progress for block production

* Update the usage of new metric

* Add block produciton test code

* Add dashboard panels for the block processing

* Revert test run code

* Fix a type in perf test

* Fix dashboard

* Fix the code feedback

* Convert multi-label metric to single label

* Remove the buckets

* Add special bucket values
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.13.0 🎉

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.

5 participants