Skip to content

Conversation

dapplion
Copy link
Collaborator

Issue Addressed

Work::BlocksBy.*Request are the only two work events using spawn_blocking_with_manual_send_idle. Its syntax prevents the use of helpful patterns like:

So far it's unclear to me why this specific work needs to use such an executor pattern. Serving blocks should be a purely i/o bound task with any long blocking CPU tasks. One expensive op dug there is forwards_iter_block_roots cloning the entire head state, but I'm unsure if it justifies the pattern.

I lack the historical context to declare this solution proper, but I'm opening this PR to trigger a conversation.

Proposed Changes

Use spawn_async in ByRoot handling workers

Copy link
Member

@ethDreamer ethDreamer left a comment

Choose a reason for hiding this comment

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

ohhhh now I see what u meant by separate thread for the consumer.. good catch!

LGTM

@realbigsean
Copy link
Member

@ethDreamer pointed out the logging changes slightly - we exit early on an error, prior to logging how many requests we sent. The same changed in this PR: #5556

mark said he'd make a follow up PR for this though

@realbigsean
Copy link
Member

For some reason, changing the Work enum in this PR caused a lint to fail. The changes here shouldn't affect the size of the enum, because we're just moving from Box to Pin<Box> in a variant. Looks like the enum was already big, but for some reason clippy didn't notice until the enum changed. This was the size before, there were two large variants:

print-type-size type: `Work<types::MainnetEthSpec>`: 192 bytes, alignment: 8 bytes
print-type-size     variant `GossipAggregate`: 192 bytes
print-type-size         field `.aggregate`: 160 bytes
print-type-size         field `.process_individual`: 16 bytes
print-type-size         field `.process_batch`: 16 bytes
print-type-size     variant `GossipAttestation`: 184 bytes
print-type-size         padding: 8 bytes
print-type-size         field `.attestation`: 144 bytes, alignment: 8 bytes
print-type-size         field `.process_individual`: 16 bytes
print-type-size         field `.process_batch`: 16 bytes

In this commit, I boxed the large unboxed fields in these variants 52777c9 , which reduced the size

print-type-size type: `Work<types::MainnetEthSpec>`: 64 bytes, alignment: 8 bytes

@realbigsean
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Apr 12, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at b6a1c86

@realbigsean realbigsean added the ready-for-merge This PR is ready to merge. label Apr 12, 2024
mergify bot added a commit that referenced this pull request Apr 12, 2024
@mergify mergify bot merged commit b6a1c86 into sigp:unstable Apr 12, 2024
@dapplion dapplion deleted the byroots-spawn branch April 13, 2024 00:22
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants