Conversation
frankmcsherry
force-pushed
the
exert_logic_config
branch
from
718015a to
bd12951
Compare
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
Looks good! Left some comments inline.
|
|
||
| // Having performed all of our work, if more than one batch remains reschedule ourself. | ||
| if !self.reduced() { | ||
| if !self.exert_effort().is_some() { |
There was a problem hiding this comment.
| if !self.exert_effort().is_some() { | |
| if self.exert_effort().is_none() { |
There was a problem hiding this comment.
Ooh, I think this is wrong both ways, actually. We want to reschedule if there is exert effort to apply. Oops. Will fix.
| @@ -140,5 +140,17 @@ impl Config { | |||
| pub fn configure(config: &mut timely::WorkerConfig, options: &Config) { | |||
| if let Some(effort) = options.idle_merge_effort { | |||
| config.set("differential/idle_merge_effort".to_string(), effort); | |||
There was a problem hiding this comment.
I don't think we're referencing idle_merge_effort anymore, so we should probably remove it here, and extend options to also take a default_excert_logic.
There was a problem hiding this comment.
I think I'm up for waiting just a bit to see how this looks before locking things in. I wrote a bunch of code before realizing that it all needs to cross thread boundaries, and if it turns out that it needs to be SerDe also .. what a mess. :D
frankmcsherry
force-pushed
the
exert_logic_config
branch
from
25f48f2 to
6cab205
Compare
2 participants
This PR allows users to set their own "exertion logic", replacing the hardwired
reducedfunction for spines. This was the method that indicated whether there was work to perform in the absence of actual updates. This PR combines this with theeffortargument to allow logic that produces anOption<usize>indicating how many updates should be virtually applied, as a function of the shape of the spine. It's a bit of a leaky abstraction (not all traces need to have a spine, right?) but the moment at which an opinion is currently injected requires the trace abstraction.Some follow-on thoughts:
Option<usize>output tousize, and commit to0to mean "do no work, and do not reschedule".idle_merge_effortconfig to beusizerather thanisize, which I think it is because ultimately theexertfunction needs that. But it doesn't make a lot of sense to provide negative idle merge effort.Spinetype, rather than as part ofTrace, but I'm not certain how to thread the system-wide config of "do this with a spine" other than across the trace.cc: @antiguru, @teskje