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

Only load Kzg in tests if necessary and only load it once #5555

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Apr 10, 2024

Issue Addressed

Loading trusted setup file and instantiating the Kzg instance is very expensive (>80% of test time for most tests), and we're doing this quite a lot of times in tests even when we don't need them (e.g. pre-deneb tests), and in some cases they're loaded twice when mock_execution_layer is used.

This PR improves this by loading Kzg trusted setup only once at most, if necessary.

Some numbers when I run beacon chain tests locally:

  • unstable branch 180s (~17m on CI)
  • unstable + fix (current branch): 173s
  • das branch: 990s (>55m on CI)
  • das branch with this fix: 269s

The difference isn't significant on unstable, so it looks like Kzg initialisation time has increased quite significantly due the changes in the das version - there's probably more optimisation that can be done on c-kzg but we should probably avoid loading it multiple times in a single test anyway, as we run each of these beacon chain tests once per fork.

Test command used:

env FORK_NAME=deneb cargo nextest run --release --features "fork_from_env,slasher/lmdb,portable" -p beacon_chain

@jimmygchen jimmygchen marked this pull request as draft April 10, 2024 13:00
@jimmygchen
Copy link
Member Author

This only works in the beacon-chain-tests crate as the client crate needs to be updated to but I'm not sure if this is a good idea. Will investigate a bit more.

@jimmygchen jimmygchen closed this Apr 10, 2024
@jimmygchen jimmygchen reopened this Apr 11, 2024
@jimmygchen jimmygchen requested a review from pawanjay176 April 11, 2024 06:30
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed do-not-merge labels Apr 11, 2024
@jimmygchen jimmygchen marked this pull request as ready for review April 11, 2024 06:54
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for running the numbers. I'm hoping the das branch in ckzg still has room for optimisations

@realbigsean realbigsean added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 11, 2024
@realbigsean
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Apr 11, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 7e49f82

mergify bot added a commit that referenced this pull request Apr 11, 2024
@mergify mergify bot merged commit 7e49f82 into sigp:unstable Apr 11, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. test improvement Improve tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants