IOSim Timeouts API refactor and optimisation #3682
Conversation
bolt12
force-pushed
the
bolt12/io-sim-timout-optimisations
branch
2 times, most recently
from
18f639c to
aaa47cf
Compare
bolt12
requested review from
dcoutts
and removed request for
nfrisby and
EncodePanda
bolt12
force-pushed
the
bolt12/io-sim-timout-optimisations
branch
6 times, most recently
from
14f6979 to
9f30150
Compare
| @@ -639,6 +640,13 @@ data SimResult a | |||
|
|
|||
| type SimTrace a = Trace.Trace (SimResult a) SimEvent | |||
|
|
|||
| traceEvents :: SimTrace a -> [(Time, ThreadId, Maybe ThreadLabel, SimEventType)] | |||
There was a problem hiding this comment.
Do we need to move traceEvents to this module?
There was a problem hiding this comment.
No, that's something I forgot I had done, I'll revert this!
| tvar <- execNewTVar nextVid | ||
| (Just $ "<<timeout-state " ++ show (unTimeoutId nextTmid) ++ ">>") | ||
| TimeoutPending | ||
| let expiry = d `addTime` time | ||
| t = Timeout tvar nextTmid | ||
| timers' = PSQ.insert nextTmid expiry (Timer tvar) timers | ||
| thread' = thread { threadControl = ThreadControl (k t) ctl } | ||
| trace <- schedule thread' simstate { timers = timers' | ||
| , nextVid = succ nextVid | ||
| , nextTmid = succ nextTmid } |
There was a problem hiding this comment.
Were bangs removed by a rebase or do we actually want to remove them?
| let timers' = PSQ.delete tmid timers | ||
| thread' = thread { threadControl = ThreadControl k ctl } |
| let timers' = PSQ.delete tmid timers | ||
| thread' = thread { threadControl = ThreadControl k ctl } | ||
| written <- execAtomically' (runSTM $ writeTVar tvar TimeoutCancelled) | ||
| (wakeup, wokeby) <- threadsUnblockedByWrites written |
bolt12
force-pushed
the
bolt12/io-sim-timout-optimisations
branch
2 times, most recently
from
240a7e7 to
e32aeef
Compare
| -- | Associated data type for 'timeout' API | ||
| -- |
There was a problem hiding this comment.
This doesn't really say anything that the syntax doesn't already say. Try and phrase it in terms of what it is for. Also note that it's not used with timeout, it's used with the "fancy" timeout API. Perhaps better to name them explicitly, e.g.
-- | The type of the timeout handle, used with 'newTimeout', 'readTimeout', 'updateTimeout' and 'cancelTimeout'.
| data TimerCompletionInfo s = | ||
| Timer !(TVar s TimeoutState) | ||
| | TimerRegisterDelay !(TVar s Bool) | ||
| | TimerThreadDelay !ThreadId |
| @@ -332,38 +335,80 @@ schedule !thread@Thread{ | |||
| let !t = NegativeTimeout nextTmid | |||
| !expiry = d `addTime` time | |||
| !thread' = thread { threadControl = ThreadControl (k t) ctl } | |||
| trace <- schedule thread' simstate { nextTmid = succ nextTmid } | |||
| !trace <- schedule thread' simstate { nextTmid = succ nextTmid } | |||
There was a problem hiding this comment.
I don't think we want to bang the result here. It is supposed to yield the trace lazily. That's why we use the lazy ST monad here.
But even if we do want to change it, it should not be done in this commit, since this commit is about something else, and it's better to have one logical change per commit.
| {-# SCC "schedule.UpdateTimeout" #-} do | ||
| let !timers' = PSQ.delete tmid timers | ||
| !thread' = thread { threadControl = ThreadControl k ctl } | ||
| trace <- schedule thread' simstate { timers = timers' } | ||
| !trace <- schedule thread' simstate { timers = timers' } |
There was a problem hiding this comment.
Same with all of these: I'm not convinced they should be strict, but even if it should, that should be in its own patch, with a commit that explains why.
| (!wakeup, wokeby) <- threadsUnblockedByWrites written | ||
| mapM_ (\(SomeTVar var) -> unblockAllThreadsFromTVar var) written | ||
| (wakeup, wokeby) <- threadsUnblockedByWrites written | ||
| !_ <- mapM_ (\(SomeTVar var) -> unblockAllThreadsFromTVar var) written |
There was a problem hiding this comment.
Is this necessary? Again, if so, separate patch.
| let (unblocked, | ||
| simstate') = unblockThreads wakeup simstate | ||
| -- Check all fired threadDelays | ||
| let !wakeupThreadDelay = mapMaybe threadDelayThreadId fired |
There was a problem hiding this comment.
For this I''d just use a list comprehension, rather than a separate named function. e.g.
let wakeupThreadDelay = [ tid | TimerThreadDelay tid <- fired ]
Note also that because it's a list, there's no point banging it.
| NewTimeout :: DiffTime -> (Timeout (IOSim s) -> SimA s b) -> SimA s b | ||
| NewRegisterDelay :: DiffTime -> (TVar s Bool -> SimA s b) -> SimA s b | ||
| NewThreadDelay :: DiffTime -> SimA s b -> SimA s b | ||
|
|
||
| UpdateTimeout :: Timeout (IOSim s) -> DiffTime -> SimA s b -> SimA s b | ||
| CancelTimeout :: Timeout (IOSim s) -> SimA s b -> SimA s b |
There was a problem hiding this comment.
Just as a grouping thing: I'd keep the NewTimeout, UpdateTimeout, CancelTimeout together in one group, since they're one API together.
The NewRegisterDelay and NewThreadDelay are each independent timer/timeout/delay features and APIs.
| | EventThreadDelay TimeoutId Time | ||
| | EventRegisterDelayCreated TimeoutId TVarId Time |
There was a problem hiding this comment.
But note that they don't have their own expiry/end events. They use the same EventTimerExpired event. Arguably they should use their own. At least, EventThreadDelay should use its own, and would not need any TimeoutId since it happens only in the context of a single thread. The EventRegisterDelayCreated does indeed still need the TimeoutId since it's based on an independent TVar rather than being tied to a single thread.
| @@ -125,7 +123,10 @@ data TimerCompletionInfo s = | |||
| Timer !(TVar s TimeoutState) | |||
| | TimerRegisterDelay !(TVar s Bool) | |||
| | TimerThreadDelay !ThreadId | |||
| | TimerTimeout !ThreadId !TimeoutId | |||
There was a problem hiding this comment.
So my suggestion is to put the lock in here, as a STVar s Bool.
There was a problem hiding this comment.
I actually prefer the solution I went for, I think it is simpler and avoids having to use STM for handling timeouts inside IOSim, do you think it's worth to change?
bolt12
force-pushed
the
bolt12/io-sim-timout-optimisations
branch
from
e32aeef to
cea2d8e
Compare
bolt12
force-pushed
the
bolt12/io-sim-timout-optimisations
branch
from
cea2d8e to
4b99a9b
Compare
|
A test failed on CI with: |
|
that's from the |
|
Ach, you're right. I don't think we can merge it until the |
|
Closing this PR in favor of input-output-hk/io-sim#9 |
3820: Fixed KeepAlive Convergence test r=bolt12 a=bolt12 # Description This small PR just extracts a particular commit from #3682 since now the actual PR lives in input-output-hk/io-sim#9. Co-authored-by: Armando Santos <[email protected]>
This PR implements and optimization to the timeout API, due to observable bottleneck on large Diffusion tests simulations.
NOTE: This PR is missing the IOSimPOR counterpart.
Benchmarks
Current master branch results
HEAD results