Skip to content

IOSim Timeouts API refactor and optimisation#9

Merged
coot merged 5 commits into
mainfrom
bolt12/io-sim-timout-optimisations
Nov 9, 2022
Merged

IOSim Timeouts API refactor and optimisation#9
coot merged 5 commits into
mainfrom
bolt12/io-sim-timout-optimisations

Conversation

@bolt12
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 IntersectMBO/io-sim#9.



Co-authored-by: Armando Santos <armando@well-typed.com>
@bolt12
Copy link
Copy Markdown
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
Comment thread 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
Copy Markdown
Collaborator

@coot coot Jul 14, 2022

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
Copy Markdown
Contributor Author

@bolt12 bolt12 Jul 14, 2022

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
Copy Markdown
Collaborator

@coot coot Jul 18, 2022

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
Copy Markdown
Contributor Author

@bolt12 bolt12 Aug 2, 2022

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
Copy Markdown
Collaborator

@coot coot Aug 5, 2022

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.

Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs
coot
coot reviewed Jul 14, 2022
View reviewed changes
Comment thread io-sim/test/Test/Control/Monad/IOSimPOR/Test.hs Outdated
@coot
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
@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
Closed
coot
coot requested changes Oct 3, 2022
View reviewed changes
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
@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
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs
@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
Copy Markdown
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.

Comment thread io-sim/test/Test/Control/Monad/IOSim/Test.hs Outdated
Comment thread io-sim/test/Test/Control/Monad/IOSimPOR/Test.hs Outdated
Comment thread io-sim/test/Test/Control/Monad/Common.hs Outdated
@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
Copy Markdown
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
Copy Markdown
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

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement IOSim timeout primitives

3 participants

@bolt12 @coot @dcoutts