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.

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

IOSim's MVars #10

Merged
merged 2 commits into from
Aug 18, 2022
Merged

IOSim's MVars #10

merged 2 commits into from
Aug 18, 2022

Conversation

coot
Copy link
Collaborator

@coot coot commented Jun 21, 2022

MVar's for IOSim

@coot coot self-assigned this Jun 21, 2022
@coot coot force-pushed the coot/mvar branch 2 times, most recently from 94d2f68 to c65fa2f Compare August 11, 2022 09:33
@coot coot marked this pull request as ready for review August 11, 2022 15:57
@coot coot requested a review from dcoutts August 11, 2022 16:32
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

Looks good! Just some minor comments.

GHC Timeout API uses MVars in its implementation, mine #9 does not because at the time there wasn't a suitable MVar definition in IOSim. I don't think I will change my PR to use MVars because these ones are based in STM and the whole purpose of my PR is to stay away from STM. My implementation mimics MVar behavior in IOSim so it should be good enough for now... I am not sure what @dcoutts is going to say about these MVars but a truly faithful MVar implementation should be specific to IOSim and thus implemented there, right?

io-classes/src/Control/Monad/Class/MonadMVar.hs Outdated Show resolved Hide resolved
io-classes/src/Control/Monad/Class/MonadMVar.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/Class/MonadMVar.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/Class/MonadMVar.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/Class/MonadMVar.hs Outdated Show resolved Hide resolved
@bolt12
Copy link
Contributor

bolt12 commented Aug 16, 2022

Also stylish is failing in CI

@coot
Copy link
Collaborator Author

coot commented Aug 16, 2022

I don't think I will change my PR to use MVars

You're using the io-sim TVar's there, isn't it? The advantage of these MVar's is that they are provide fairness, unlike TMVar's.

btw, this implementation relies on @dcoutts idea.

@bolt12
Copy link
Contributor

bolt12 commented Aug 16, 2022

You're using the io-sim TVar's there, isn't it? The advantage of these MVar's is that they are provide fairness, unlike TMVar's.

btw, this implementation relies on @dcoutts idea.

I was, now it is using STRef's. How does STM provide fairness, I remember seeing that STM doesn't exactly guarantee fairness like MVars do

@coot
Copy link
Collaborator Author

coot commented Aug 16, 2022

How does STM provide fairness, I remember seeing that STM doesn't exactly guarantee fairness like MVars do

STM doesn't provide fairness, that's why the dance with MVarState and both putMVar and takeMVar are not implemented using a single atomic STM action.

coot and others added 2 commits August 17, 2022 09:10
Including a default implementation using 'MonadSTM', which guarantees
fairness.

Co-authored-by: Duncan Coutts <dcoutts@users.noreply.github.com>
@bolt12
Copy link
Contributor

bolt12 commented Aug 17, 2022

STM doesn't provide fairness, that's why the dance with MVarState and both putMVar and takeMVar are not implemented using a single atomic STM action.

So when you say:

Including a default implementation using 'MonadSTM', which guarantees fairness.

what do you mean by fairness? what I understand by fairness here is that if you have various thread blocked on waiting for a take in a loop, and one thread putting in the MVar in a loop, eventually all threads will be able to acquire the take. I do not think STM guarantees this fairness

@coot
Copy link
Collaborator Author

coot commented Aug 17, 2022

what do you mean by fairness?

It means blocked putMVars (or takeMVars) are served in FIFO order. STM indeed does not guarantee that, and that's why putMVar and takeMVar are more complex than what TMVar would provide, also note that we need two atomically blocks not a single one! Does this explains it well?

@coot
Copy link
Collaborator Author

coot commented Aug 17, 2022

The same fairness (FIFO order) is guaranteed by MVar's provided by base: ref.

@bolt12
Copy link
Contributor

bolt12 commented Aug 17, 2022

@coot I understand now! Thank you! :D

@coot coot merged commit 3580d08 into main Aug 18, 2022
@coot coot deleted the coot/mvar branch August 18, 2022 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants