backing: refactor off of jobs system#5377
Conversation
rphmeier
added
A0-please_review
| for deactivated in update.deactivated { | ||
| jobs.remove(&deactivated); | ||
| } |
There was a problem hiding this comment.
This is almost all we need for async backing - just not deactivating stuff quite as quickly.
| Some(a) => a, | ||
| }; | ||
|
|
||
| macro_rules! try_runtime_api { |
There was a problem hiding this comment.
Everything from here onwards has been copied more or less verbatim from the util::JobTrait implementation that I've removed.
While there are improvements to be made, I will not be addressing them in this PR.
| loop { | ||
| futures::select! { | ||
| validated_command = self.background_validation.next() => { | ||
| let _span = span.child("process-validation-result"); |
There was a problem hiding this comment.
this span wasn't useful so you won't find it in the refactored code
| Some(msg) => { | ||
| // we intentionally want spans created in `process_msg` to descend from the | ||
| // `span ` which is longer-lived than this ephemeral timing span. | ||
| let _timing_span = span.child("process-message"); |
There was a problem hiding this comment.
this span wasn't useful so you won't find it in the refactored code
node/core/backing/src/lib.rs
Outdated
| let mut jobs = HashMap::new(); | ||
|
|
||
| // TODO [now]: refactor to JFYIError and add a `run_until_error` function | ||
| // which actually does this loop |
There was a problem hiding this comment.
are you planning to address it here or in a follow-up?
There was a problem hiding this comment.
Here (I have a TODO in the description)
|
I didn't really look deeply since it the code was mostly copied. Looks good overall. One thing we might improve in a follow-up is taking Good to go once it's fully tested on Versi. |
drahnr
left a comment
drahnr
left a comment
There was a problem hiding this comment.
Generally looks good, I'd prefer not passing Context to inner functions unless strictly necessary.
|
bot merge |
|
Waiting for commit status. |
3 participants
Part of #4259
This refactors the candidate-backing subsystem not to use the util::jobs framework.
candidate-backing was one of the first subsystems, and it was written in an object-oriented style (as opposed to the functional style which is better for asynchronous code). This meant that some functions had to be changed from
&selfto&mut selfeven though it doesn't seem like they need to be - otherwise it would requireContext: Sync.This also changed the behavior of the subsystem overall: previously, each relay-parent's work was spawned in a background task by
util::Jobsbut now we do all the work in a single task with the exception of actual PoV recovery and candidate validation - that still happens in the background. There's not much heavy work happening in the 'main thread' of candidate backing except for signing messages occasionally (a few for every block), so this shouldn't have a meaningful impact on performance.The goal of this refactoring is to make it easier to control when candidate-backing 'jobs' actually get cleaned up when we implement #3779 (In particular, #5057, which has more details)
TODO:
fatalitycrate as is done in other subsystems.runintorunandrun_until_error, as is done in other subsystems.