-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[Perf] Include json serialisation in the get_blocks
blocking task
#3253
Conversation
get_blocks
blocking taskget_blocks
blocking task
Opened for review as the node crash was due to devnet running with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is likely to decrease the number of requests that can be supported by a node before a stack overflow occurs.
However, if a node operator enforces the default REST limits, this should be mitigated. This however should be backed up with explicit testing.
+1 on the testing for establishing robust rate limits but I think that tradeoff should be considered separately from this change. Blocking an async worker with hundreds of milliseconds worth of serialisation would impact the rest of the node's resource allocation and execution. |
…rest"" This reverts commit 7dbaadd.
A small follow-up to #3252. Devnet testing of the endpoint revealed serialisation times in the milliseconds (usually hundreds), motivating the change in this PR.
I managed to OOM some of the nodes with this change, the root cause is still tbd but I suspect it's also present in #3252. This is likely because previously blocked workers are no longer artificially throttling execution. Multiple concurrent requests for MBs of data could then lead to issues despite the existing rate limiting.
Drafted until we can figure out why.