refactor: split async/sync work in stages #4636
Merged
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
onbjerg
added
A-staged-sync
onbjerg
force-pushed
the
onbjerg/stage-poll-ready
branch
from
e47ed97 to
2f582ed
Compare
github-actions
bot
added
the
S-stale
rkrasiuk
added
M-prevent-stale
rkrasiuk
force-pushed
the
onbjerg/stage-poll-ready
branch
from
2f582ed to
661876f
Compare
rkrasiuk
requested review from
rkrasiuk,
mattsse,
Rjected,
rakita,
joshieDo,
shekhirin and
gakonst
as code owners
shekhirin
reviewed
Nov 15, 2023
mattsse
requested changes
Nov 15, 2023
rkrasiuk
force-pushed
the
onbjerg/stage-poll-ready
branch
from
4ffb880 to
0a456c8
Compare
mattsse
approved these changes
Nov 17, 2023
| { | ||
| input.checkpoint = Some(stage_progress); | ||
| loop { | ||
| poll_fn(|cx| exec_stage.poll_execute_ready(cx, input)).await?; |
There was a problem hiding this comment.
we add an extension trait with a helper function that does this:
https://docs.rs/tower/latest/tower/trait.ServiceExt.html#method.ready
but perhaps not worth it, maybe in a followup
| pub consensus: Arc<dyn Consensus>, | ||
| downloader: D, | ||
| /// Block response buffer. | ||
| buffer: Vec<BlockResponse>, |
There was a problem hiding this comment.
these could probably be an Option, right?
with the assumption that calling execute before ready can panic
but this is fine
Comment on lines
+38
to
+39
| /// Database handle. | ||
| provider: Provider, |
There was a problem hiding this comment.
I like this a lot, no more fiddling with the db types directly,
we could also box this I think
…, add header sync gap provider
rkrasiuk
force-pushed
the
onbjerg/stage-poll-ready
branch
from
1cc5a4c to
9bb2e44
Compare
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.
This is essentially just #4215 but renamed to better reflect what the PR will now be about.
Previously, most of the work in #4215 was to try and remove the
Send + Syncbounds forDbTxwhere possible to make it work with redb, however the transactions have since been madeSend + Syncupstream so that is no longer needed.Instead, this will continue work on splitting async and sync work in the stages into two parts, inspired by
tower'sServicetrait: async work will be done inpoll_ready, which will tell the caller when the stage is ready to be executed, and sync work (incl. writing to the database) will be performed inexecuteas normal.This will make it a lot clearer when things block and when they do not; most stages block in
executeand do not really leverage async at all, some notable exceptions being the header and bodies stage.Assumptions
poll_readyis only required to be called beforeexecute(Rebasing newer work and pushing in a bit)