-
Notifications
You must be signed in to change notification settings - Fork 23
si-timers: round difftime to microseconds in IOSim #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new function roundDiffTimeToMicroseconds to round DiffTime values down to microsecond precision and uses it in the SI.MonadDelay instance for IOSim. This ensures that thread delays in the SI timer interface have consistent microsecond granularity without fractional microseconds.
- Added
roundDiffTimeToMicrosecondsfunction to roundDiffTimeto microseconds - Updated
SI.MonadDelayinstance to use rounding instead of passing through the originalDiffTime - Added property tests to verify the rounding behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| io-classes/si-timers/src/Control/Monad/Class/MonadTimer/SI.hs | Implements roundDiffTimeToMicroseconds function that rounds down to microsecond precision |
| io-sim/src/Control/Monad/IOSim/Types.hs | Updates SI.MonadDelay instance to apply microsecond rounding to thread delays |
| io-classes/si-timers/test/Test/MonadTimer.hs | Adds property tests for the new rounding function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
132b39d to
d8ef728
Compare
nfrisby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested a possible alternative
|
|
||
| -- | Round to microseconds. | ||
| -- | ||
| -- For negative diff times it rounds towards negative infinity, which is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does threadDelay x for x<0 have a different behavior than threadDelay 0?
If not, then this comment seems like a distraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
threadDelay doesn't, but timeout does, I am not sure about registerDelay, probably not, I'll check.
| instance SI.MonadDelay (IOSim s) where | ||
| threadDelay d = | ||
| IOSim $ oneShot $ \k -> ThreadDelay d (k ()) | ||
| IOSim $ oneShot $ \k -> ThreadDelay (SI.roundDiffTimeToMicroseconds d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evaluate the rounding calculation strictly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same question applies several other places in this diff.
| ) => MonadDelay m where | ||
| -- | All instances SHOULD round delays down to the nearest microsecond so the | ||
| -- behaviour matches the `IO` instance. | ||
| threadDelay :: DiffTime -> m () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of leaving the type as more generous than the canonical implementation's (ie IO), what do you think about instead adding a bespoke newtype?
import Data.Fixed (Micro)
newtype DiffTimeRoundedToMicroseconds = DiffTimeRoundedToMicroseconds Micro
deriving (...?)
roundDiffTimeToMicroseconds :: DiffTime -> DiffTimeRoundedToMicroseconds
forgetDiffTimeToMicroseconds :: DiffTimeRoundedToMicroseconds -> DiffTime
This would make everything very explicit.
The disadvantage is that people would sometimes be forced to call roundDiffTimeToMicroseconds even when they were certain their DiffTime's internal Pico was already a whole number multiple of 1e6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change the API in si-timers and IOSim to use DiffTimeRoundedTOMicroseconds it will have massive consequences in ouroboros-newtork. The cost of changing it might be too big.
The question is, how surprised will one be with threadDelay 0.000_000_1s only to yield the current thread for 0s: it will put it in the back of the scheduler's queue, instead of executing it in another time slot - this could be a bit surprising.
threadDelay negativeDelay doesn't yield a thread, but rounding doesn't change non-negative numbers into negative ones, so no surprises in this case :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| solution | conseqences | |
|---|---|---|
| 1. | DiffTimeRoundedToMicroseconds |
breaking API, but no surprises |
| 2. | implicit rounding | surprises in IOSim, but no surprises when compared to IO |
| 3. | the current situation | no surprises in IOSim, but different semantics than IO |
My pick would be: 1. long-term, 2. or 3. short-term.
This makes `threadDelay` and `MonadTimer` from `si-timers` sublibrary behave the same way for `IOSim` and `IO`.
d8ef728 to
cce1b61
Compare
This makes
threadDelayfromsi-timerssublibrary behave the same wayfor
IOSimandIO.