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

Avoid computing rpc_blob_limits multiple times #6595

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

pawanjay176
Copy link
Member

Issue Addressed

N/A

Proposed Changes

rpc_blob_limits was being computed repeatedly even though its a constant value. Move it to a LazyLock block to avoid repeated computation.
Found this taking ~10% of cpu when I was playing around with flamegraph

@pawanjay176 pawanjay176 added low-hanging-fruit Easy to resolve, get it before someone else does! v6.0.0 New major release for hierarchical state diffs labels Nov 19, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Nice find!

@michaelsproul
Copy link
Member

The test failure looks related. It might be because E: EthSpec has been fixed to MainnetEthSpec? We could one lazy lock per spec if necessary

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. optimization Something to make Lighthouse run more efficiently. labels Nov 19, 2024
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

nice find! what else showed up in the flamegraph?

@pawanjay176
Copy link
Member Author

pawanjay176 commented Nov 19, 2024

Moved rpc_tests to run over MainnetEthSpec. I think it made a difference when minimal blob sizes were small. Don't think it makes much of a difference in terms of test speeds now. Not sure if there's any reason to run them over minimal cc @AgeManning for when you are back

@pawanjay176
Copy link
Member Author

Not sure if there's any reason to run them over minimal
We need it for the simulator tests. Running sim tests over MainnetEthSpec will take 8x longer. Can revisit this if we manage to replace sim tests with assertoor tests. Reverted back to minimal in rpc_tests

@pawanjay176
Copy link
Member Author

what else showed up in the flamegraph?

Mostly signature verification and decompression taking up cpu time. Milhouse uses a lot of stack depth. Both are expected I guess.

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Nov 19, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 20, 2024
@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Nov 20, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 6e1945f

mergify bot added a commit that referenced this pull request Nov 20, 2024
@mergify mergify bot merged commit 6e1945f into sigp:unstable Nov 20, 2024
29 checks passed
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
* Compute blob rpc limits in static block

* Fix min size

* Use MainnetEthSpec in rpc tests

* Revert MainnetEthSpec; add another constant for blob size minimal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low-hanging-fruit Easy to resolve, get it before someone else does! optimization Something to make Lighthouse run more efficiently. ready-for-merge This PR is ready to merge. v6.0.0 New major release for hierarchical state diffs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants