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

Improve forkchoice updateHead #2747

Closed
twoeths opened this issue Jun 24, 2021 · 9 comments
Closed

Improve forkchoice updateHead #2747

twoeths opened this issue Jun 24, 2021 · 9 comments
Labels
prio-low This is nice to have. scope-performance Performance issue and ideas to improve performance.

Comments

@twoeths
Copy link
Contributor

twoeths commented Jun 24, 2021

Describe the bug

This is from a prater node with no validator
Screen Shot 2021-06-24 at 09 00 35
0624_prater_forkchoice_updateHead.cpuprofile.zip

a normal state transition takes 227.6m, 212.2 ms is for updateHead

We need to find a way to improve computeDeltas in forkchoice implementation.

Expected behavior

Not sure what the expectation is but I think forkchoice is designed to be lightweight, taking 90% of time per normal state transition for forkchoice is too much.

@twoeths
Copy link
Contributor Author

twoeths commented Jun 24, 2021

I think one of the cost is the slowness of BigInt calculation in computeDeltas. One idea is to track effectiveBalance as number instead of BigInt since MAX_EFFECTIVE_BALANCE=32000000000 which is still in the range of number.

We need to write benchmark test for any solutions we want to go.

@dapplion dapplion added the scope-performance Performance issue and ideas to improve performance. label Jun 24, 2021
@dapplion
Copy link
Contributor

Related #2455

@g11tech
Copy link
Contributor

g11tech commented Jul 6, 2021

@dapplion assign pls

@dapplion dapplion added the prio-low This is nice to have. label Jul 6, 2021
@dapplion
Copy link
Contributor

dapplion commented Jul 6, 2021

Screenshot from 2021-07-06 18-54-40
Screenshot from 2021-07-06 18-54-50

After implementing head caching fork-choice runs take 0.5% of CPU time in a not overloaded node (no subnets). Worth to look into it but not an high priority.

Also note that metrics reads match what Tuyen got in the profile.

@twoeths
Copy link
Contributor Author

twoeths commented Jul 14, 2021

As noted in #2332, effectiveBalance is always multiple of EFFECTIVE_BALANCE_INCREMENT so ideally we want to track the weight as 32, 31, 30 instead of 32000000000, 31000000000, 30000000000

@dapplion
Copy link
Contributor

Current metrics the fork-choice cost is very low 👍

@dapplion
Copy link
Contributor

Fork-choice updateHead cost is about ~1% of total CPU time currently. I think it can still be improved, but it's low priority

@dapplion
Copy link
Contributor

Below is a CPU profile of running updateHead() in a loop inside a benchmark
image

As shown in other profiles it's clear than the vast majority of time is spent on computeDeltas. The below percentages are likely wrong since for some reason the stack traces appear as the stack frame bounces from updateHead to updateHead/computeDeltas.

image

Reviewing computeDeltas, it's a pretty simple function:

  • Loop over votes
  • for each vote resolve what's voting to and add / substract its weight from those nodes

@twoeths
Copy link
Contributor Author

twoeths commented Aug 20, 2023

closing via #5882

@twoeths twoeths closed this as completed Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio-low This is nice to have. scope-performance Performance issue and ideas to improve performance.
Projects
None yet
Development

No branches or pull requests

3 participants