Reduce size of futures in HTTP API to prevent stack overflows #5104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelsproul
added
bug
jxs
reviewed
Jan 22, 2024
| @@ -866,14 +866,14 @@ where | |||
| panic!("Should always be a full payload response"); | |||
| }; | |||
|
|
|||
| let signed_block = block_response.block.sign( | |||
| let signed_block = Arc::new(block_response.block.sign( | |||
There was a problem hiding this comment.
is this test only code? If so do we also have stack overflows during tests?
(Question applies to all the uses in this file)
There was a problem hiding this comment.
yeah this is just for tests, I ended up having to make quite a few changes to make the types consistent. There's a bit of inefficient Arc-ing and de-Arc-ing, but it's just in tests so it shouldn't be too bad
Jimmy got some stack overflows in tests, but only in debug mode
| @@ -380,6 +379,10 @@ pub async fn reconstruct_block<T: BeaconChainTypes>( | |||
| None | |||
| }; | |||
|
|
|||
| // Perf: cloning the block here to unblind it is a little sub-optimal. This is considered an | |||
| // acceptable tradeoff to avoid passing blocks around on the stack (unarced), which blows up | |||
There was a problem hiding this comment.
Suggested change
| // acceptable tradeoff to avoid passing blocks around on the stack (unarced), which blows up | |
| // acceptable tradeoff to avoid passing blocks around on the stack (unArc'ed), which blows up |
There was a problem hiding this comment.
I kinda like the lowercase spelling
paulhauner
approved these changes
Jan 23, 2024
|
I'll merge this because there doesn't seem to be anything substantial outstanding and @michaelsproul said it's ready for "review/merge". |
This was referenced Jan 23, 2024
danielramirezch
pushed a commit
to danielramirezch/lighthouse
that referenced
this pull request
Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Closes #5080.
Proposed Changes
Box::pinon a few futures inpublish_block. This reduces the size of related futures from 100KB to ~33KB.Arcs to reduce the amount of data on the stack inpublish_blockand related functions. This reduces the size of the publish block futures from ~33KB to ~16KB.One of the reasons for the stack size blowup in futures is described in this blog post: https://swatinem.de/blog/future-size/.
Even with that knowledge, I still found it quite hard to determine interventions that would be effective, and did a lot of trial and error changing things and re-running the type-size analysis.
The commands I used for testing were:
Followed by post-processing:
Additional Info
There's still more we could do here. Some types like the
PreProcessingSnapshotend up on the stack during block publishing, and are quite large due to containing unarced blocks and states.There's also an outstanding mystery as to how 100KB futures can cause an 8MB stack to overflow, when only they should only be nested ~1 at a time. Either I've misunderstood what happens when futures poll sub-futures recursively, or there's something in
warpthat creates the amplification.NOTE: I have yet to measure whether the boxing and arcing has an impact on performance. It would be good to monitor metrics for block publication time after deploying this branch.
A backtrace implicating
warpcan be found here: https://gist.github.com/michaelsproul/d0a4b93bb9d21a5be1103acf4c0a3753.Thanks to @dapplion for pairing on this and helping with some of the early progress.