add time skipping to chasm framework - #10934
Conversation
ecc8e5e to
e1b71ce
Compare
| @@ -0,0 +1,95 @@ | |||
| package chasm | |||
There was a problem hiding this comment.
for reviewers:
- this is the base branch for a stack of small PRs that makes things easy to be reviewed
- the whole pr doesn't include execution implementation, and it is chasm framework only
| // signal. Nil-safe. A transition without a current time is never valid — every meaningful field is | ||
| // derived relative to the current time, so without it there is nothing to apply. | ||
| func (t *TimeSkippingTransition) IsValid() bool { | ||
| return t != nil && !t.CurrentTime.IsZero() && (!t.TargetTime.IsZero() || t.DisabledAfterFastForward) |
There was a problem hiding this comment.
Based on these condition, NewTimeSkippingTransition returns an invalid transition. Is this the desired behavior?
There was a problem hiding this comment.
yes, it returns an empty transition for users to set a target time in the future. I feel isValid or this New method is not a name good enough to indicate this the transition has been correctly set.
There was a problem hiding this comment.
solved by rebasing the newest main
| // present by TrackEarliestFutureTime) and disable time skipping — the budget is reached. | ||
| // This is what lets the budget cap a chain of runs: a run with no earlier candidate | ||
| // consumes the remaining budget by skipping to the fast-forward and disabling. | ||
| t.TrackEarliestFutureTime(ffTargetTime) |
There was a problem hiding this comment.
TrackEarliestFutureTime already does the checks in L86.
Wouldn't it be better if TrackEarliestFutureTime returned a bool indicating if TargetTime was replaced, and then here you could replace L86-L94 with if t.TrackEarliestFutureTime(...) { t.DisableAfterFastForward = true }
Also, based on this code, if ffTargetTime is already the same time as t.TargetTime, the function t.TrackEarliestFutureTime replaces t.TargetTime (which is no-op), and you are setting t.DisableAfterFastForward = true. Is this what you want?
There was a problem hiding this comment.
for the second point, if they are equal, fastforward should be disabled, also mentioned above, the allowed budget has been reached right?
There was a problem hiding this comment.
if they are equal, fastforward should be disabled
yes
There was a problem hiding this comment.
I believe the first version TrackEarliestFutureTime returned a boolean, but I removed it as this method may become one needed by component users (whenever a component user wants to override the default handler of findNextTargetTime) and I don't want users to think about how to handle the result and they just set all possible time points. Compared to that benefit, I think adding this two lines are trivial redundancy.
There was a problem hiding this comment.
solved by rebasing the newest main
| if ms.executionInfo.GetTimeSkippingInfo() == nil { | ||
| // init/update only return an error when their init-or-not precondition is violated, | ||
| // which the dispatch below already guarantees; the error is redundant here. | ||
| _ = ms.initTimeSkippingInfo(config, nil, 0) |
There was a problem hiding this comment.
Do we have any guarantees that no other kind of error could be returned? What if initTimeSkippingInfo implementation changes? ditto below with updateTimeSkippingInfo
There was a problem hiding this comment.
Actually I am thinking to remove the errors returned by init/update completely after I change in workflows to use the SetTSI as well once this is merged. And have added a todo for it.
There was a problem hiding this comment.
solved by rebasing the newest main
e0719b1 to
22c69cb
Compare
59fb047 to
cf53df6
Compare
cf53df6 to
f65f311
Compare
fcff499 to
0cd1ed6
Compare
0cd1ed6 to
83df579
Compare
3 participants
What changed?
add time skipping to chasm framework
Stack
How did you test it?