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

Use newer version of io-sim #4047

Merged
merged 2 commits into from
Oct 6, 2022
Merged

Use newer version of io-sim #4047

merged 2 commits into from
Oct 6, 2022

Conversation

coot
Copy link
Contributor

@coot coot commented Sep 27, 2022 •
edited
Loading

Description

Update to newer version of io-sim (see input-output-hk/io-sim#24).

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@coot
Copy link
Contributor Author

coot commented Sep 27, 2022

Requires both:

@coot coot force-pushed the coot/update-io-sim branch 3 times, most recently from 66254b4 to c86b1c8 Compare September 27, 2022 10:29
@coot coot marked this pull request as ready for review September 27, 2022 11:01
@coot coot requested review from newhoggy and a team as code owners September 27, 2022 11:01
dnadales
dnadales approved these changes Sep 28, 2022
View reviewed changes
Copy link
Member

@dnadales dnadales left a comment

Choose a reason for hiding this comment

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

LGTM modulo adding some more context in the two TODO's we have introduced.

ouroboros-consensus/src/Ouroboros/Consensus/Util/MonadSTM/NormalForm.hs Outdated
(newTMVar, newTMVarIO)
import Control.Concurrent.Class.MonadSTM.Strict.TVar as StrictSTM hiding
(newTVar, newTVarIO, newTVarWithInvariantIO)
-- TODO: use strict versions of 'TQueue' and 'TBQueue'
Copy link
Member

Choose a reason for hiding this comment

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

Could we give a rationale on why this is required? Eg what would happen if we don't use the strict versions of these queue types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The queues could store thunks which could will be evaluated only once they are read from the queue. This could retain memory for longer than necessary.

Since the strict version was imported in the original version, I guess using strict queues was the intention, it used non-strict queues because they were not implemented and the Control.Monad.Class.MonadSTM.Strict module exported non-strict ones.

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 updated the todo.

ouroboros-consensus/src/Ouroboros/Consensus/Util/MonadSTM/StrictMVar.hs Outdated
@@ -5,6 +5,7 @@
{-# LANGUAGE TupleSections #-}
{-# LANGUAGE TypeFamilies #-}

-- TODO: this module ought to use 'MonadMVar', and be moved to `strict-stm`.
Copy link
Member

Choose a reason for hiding this comment

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

It'd help whoever needs to do this to provide a rationale for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two reasons:

  • make the implementation more robust (the implementation here is wrong, MVar's supposed to provide fairness, which TVar's do not provide)
  • make it a standalone package which extends io-sim (in the same way strict-stm extends it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ref: input-output-hk/io-sim#34

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 updated the todo.

@coot coot force-pushed the coot/update-io-sim branch 2 times, most recently from c9e60bc to 33bbbb0 Compare October 5, 2022 18:24
@coot
Copy link
Contributor Author

coot commented Oct 6, 2022

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 6, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit a8c9cfc into master Oct 6, 2022
@iohk-bors iohk-bors bot deleted the coot/update-io-sim branch October 6, 2022 16:03
@andreabedini andreabedini mentioned this pull request Oct 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnadales dnadales dnadales approved these changes

@bolt12 bolt12 bolt12 approved these changes

@nfrisby nfrisby Awaiting requested review from nfrisby

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@coot @dnadales @bolt12