Skip to content
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.

Sign up for GitHub

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

Jump to bottom

IOSim Timeouts API refactor and optimisation #9

Merged
merged 5 commits into from
Nov 9, 2022
Merged

IOSim Timeouts API refactor and optimisation #9

merged 5 commits into from
Nov 9, 2022

Conversation

bolt12
Copy link
Contributor

@bolt12 bolt12 commented Jun 20, 2022 •
edited by coot
Loading

This is the same as in IntersectMBO/ouroboros-network#3682 but on the new repository

Resolves IntersectMBO/ouroboros-network#3672

@bolt12
Copy link
Contributor Author

bolt12 commented Jun 20, 2022

NOTE that IntersectMBO/ouroboros-network#3682 had 1 more commit that fixed a KeepAlive convergence test. I am going to add that as a separate PR over at ouroboros-network.

This was referenced Jun 20, 2022
Closed
Merged
@bolt12 bolt12 requested a review from coot June 21, 2022 08:28
@coot
Copy link
Collaborator

coot commented Jun 22, 2022

Can we turn this PR into a draft, we need to do the IOSimPOR side before merging, isn't it?

@bolt12 bolt12 marked this pull request as draft June 22, 2022 08:23
iohk-bors bot added a commit to IntersectMBO/ouroboros-network that referenced this pull request Jun 29, 2022
@iohk-bors @bolt12
Merge #3820
847f8ac 
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 <armando@well-typed.com>
@bolt12
Copy link
Contributor Author

bolt12 commented Jul 5, 2022

closes IntersectMBO/ouroboros-network#3672

@bolt12 bolt12 force-pushed the bolt12/io-sim-timout-optimisations branch 2 times, most recently from bfe7ce3 to 56473c6 Compare July 8, 2022 15:25
@bolt12 bolt12 marked this pull request as ready for review July 8, 2022 15:25
@bolt12 bolt12 requested a review from dcoutts July 14, 2022 08:48
coot
coot reviewed Jul 14, 2022
View reviewed changes
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
, Nothing) , Just tdid) -> do
let thread'' = stepThread thread'
control' = advanceControl (threadStepId thread') control
races' = updateRacesInSimState thread' simstate
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to use updateRacesInSimState only in reschedule and deschedule functions, e.g. we only call it when we deschedule a thread. So the question is if it's necessary to have it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question, but if you look a few lines above (or in the diff before I changed stuff) when we found a suitable exception handler we call updateRacesInSimState. So I figured this particular case and the ones below should behave in the same way since in a nutshell we are dealing with particular CatchFrames (they are not exactly CatchFrames in the sense that one called catch somewhere in the code, but they are implicit catch call)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave that question for @rjmh then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is required anymore

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still like to hear back from John, but we don't need to be blocked on merging.

io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Show resolved Hide resolved
@coot
Copy link
Collaborator

coot commented Jul 14, 2022

Lets consider two concurrent threads which are blocked on threadDealy, which expires at the same time. We don't need to race the two events because there's nothing racy on its own here, if the two threads do something later that introduces a race that race can be inverted. Even if we deschedule the thread just after a timer delay expires (and thus we start a new step), this will not influence the ability to invert the following racing steps.

dcoutts
dcoutts previously requested changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got through the first two commits. Lots of comments.

io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
@bolt12 bolt12 force-pushed the bolt12/io-sim-timout-optimisations branch from 56473c6 to 65dc1a6 Compare August 2, 2022 14:03
@bolt12 bolt12 requested review from coot and dcoutts August 2, 2022 14:13
@coot coot mentioned this pull request Sep 26, 2022
Open
coot
coot requested changes Oct 3, 2022
View reviewed changes
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated Show resolved Hide resolved
@bolt12 bolt12 force-pushed the bolt12/io-sim-timout-optimisations branch from 65dc1a6 to bfa7957 Compare October 10, 2022 14:40
@bolt12 bolt12 requested a review from coot October 10, 2022 14:41
@bolt12 bolt12 force-pushed the bolt12/io-sim-timout-optimisations branch from bfa7957 to 2e2e00d Compare October 10, 2022 14:53
coot
coot reviewed Oct 13, 2022
View reviewed changes
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Internal.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim/Types.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Internal.hs Show resolved Hide resolved
@coot coot mentioned this pull request Oct 26, 2022
Closed
@bolt12 bolt12 requested a review from coot November 4, 2022 09:07
@bolt12 bolt12 force-pushed the bolt12/io-sim-timout-optimisations branch 2 times, most recently from a0b6cea to 75cf43d Compare November 4, 2022 10:06
coot
coot requested changes Nov 9, 2022
View reviewed changes
Copy link
Collaborator

@coot coot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bolt12 I think it's better to make a separate PR out of refactoring the test suite. It requires a bit more work, but we can merge the timeout part.

io-sim/test/Test/Control/Monad/IOSim/Test.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/IOSimPOR/Test.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/Common.hs Outdated Show resolved Hide resolved
@bolt12 bolt12 force-pushed the bolt12/io-sim-timout-optimisations branch from 75cf43d to 4c83fac Compare November 9, 2022 09:10
@bolt12 bolt12 requested a review from coot November 9, 2022 09:12
@bolt12
Copy link
Contributor Author

bolt12 commented Nov 9, 2022

I removed the refactor commits and move those to #47

coot
coot approved these changes Nov 9, 2022
View reviewed changes
Copy link
Collaborator

@coot coot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot @bolt12 :)

@coot coot dismissed dcoutts’s stale review November 9, 2022 20:08

The review was addressed.

@coot coot merged commit 8037962 into main Nov 9, 2022
@coot coot deleted the bolt12/io-sim-timout-optimisations branch November 9, 2022 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coot coot coot approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

Assignees
No one assigned
Labels
None yet
Projects
Ouroboros Network
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement IOSim timeout primitives
3 participants
@bolt12 @coot @dcoutts