-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix a bunch of bugs and infelicities #9
base: master
Are you sure you want to change the base?
Conversation
a449ac4
to
6b7fbaf
Compare
@crockeea, please have a look at this. |
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.
Still missing modifyMVarMasked_
, modifyMVarMasked
, and mkWeakMVar
.
Much improved overall!
case putMVar# mvar# x s# of | ||
s2# -> (# s2#, () #) | ||
#endif | ||
putMVar !mv !x = rnf x `seq` MV.putMVar mv x |
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.
putMVar !mv !x = rnf x `seq` MV.putMVar mv x | |
putMVar !mv x = rnf x `seq` MV.putMVar mv x |
Gratuitous bang?
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.
That's needed to preserve the original behavior if rnf
doesn't actually force anything.
instance NFData Foo where
rnf _ = ()
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.
What we can do is change that to putMVar !mv x = force x `seq` MV.putMVar mv x
. I guess that's better.
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.
I'm afraid that subtlety is lost on me. force x
is equivalent to rnf x `seq` x
, and that's materially different from rnf x
? I'll take your word for it.
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.
Define
data Foo
instance NFData Foo where
rnf _ = ()
f, g :: Foo -> ()
f x = force x `seq` ()
g x = rnf x `seq` ()
Then f undefined = undefined
, but g undefined = ()
.
Whether this is the "right" decision is somewhat unclear to me, but it matches the previous behavior.
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.
Thanks for that example. The documentation for rnf
says:
rnf should reduce its argument to normal form (that is, fully evaluate all sub-components), and then return '()'.
so doesn't your instance violate that? At any rate, this is indeed a pathological corner case, and I have no opinion.
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.
There are special circumstances when you might conceivably want rnf
to do nothing, as a generic way to force the spine of a data structure but not its leaves. Do we want to worry about that?
{-| | ||
Take a value from an 'MVar', put a new value into the 'MVar' and | ||
return the value taken. Note that there is a race condition whereby | ||
another process can put something in the 'MVar' after the take | ||
happens but before the put does. | ||
-} | ||
swapMVar :: NFData a => MVar a -> a -> IO a | ||
swapMVar mvar new = mask $ \_ -> do | ||
swapMVar !mvar new = seq (force new) $ mask $ \_ -> do |
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.
Shouldn't the local putMVar
suffice, like the original version?
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.
I believe we want to force before we take the MVar
. That way we don't hold the MVar
any longer than necessary. We also avoid problems if the forcing throws an exception. Once we've forced manually, we don't want to do it again implicitly, because that would be a waste of time.
a' <- Exception.catch (unmask (io a)) | ||
(\(e :: SomeException) -> do putMVar m a; throw e) | ||
putMVar m a' | ||
modifyMVar_ m io = MV.modifyMVar_ m $ io >=> \p -> force p `seq` return p |
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.
Bang on m?
modifyMVar_ m io = MV.modifyMVar_ m $ io >=> \p -> force p `seq` return p | |
modifyMVar_ !m io = MV.modifyMVar_ m $ io >=> \p -> force p `seq` return p |
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.
No need. MV.modifyMVar_
does that for us.
putMVar m a' | ||
return b | ||
|
||
modifyMVar m io = MV.modifyMVar m $ io >=> \pq -> force (fst pq) `seq` return pq |
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.
Bang on m?
modifyMVar m io = MV.modifyMVar m $ io >=> \pq -> force (fst pq) `seq` return pq | |
modifyMVar !m io = MV.modifyMVar m $ io >=> \pq -> force (fst pq) `seq` return pq |
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.
Again, no need.
case tryPutMVar# mvar# x s# of | ||
(# s, 0# #) -> (# s, False #) | ||
(# s, _ #) -> (# s, True #) | ||
tryPutMVar !mv !x = rnf x `seq` MV.tryPutMVar mv x |
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.
Gratuitous bang?
tryPutMVar !mv !x = rnf x `seq` MV.tryPutMVar mv x | |
tryPutMVar !mv x = rnf x `seq` MV.tryPutMVar mv x |
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.
Again, needed if we want to force to WHNF if rnf
does nothing.
Yes, I noticed that too... I suppose I could just shove those in here too. |
@ygale, have you had a chance to look at this yet? |
tryReadMVar :: MVar a -> IO (Maybe a) | ||
-- This is a best-effort compatibility shim for really old GHC versions. | ||
-- It's not really what you'd call *right*. | ||
tryReadMVar !m = uninterruptibleMask $ \_ -> do |
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.
Actually, mask
should be enough, since nothing in here is interruptible. An earlier draft used putMVar
. I'm not sure which version makes the least nonsense. But ultimately, it doesn't really matter much, since this is a half-broken shim. (readMVar
itself should be considered half-broken for earlier base
versions, since it used to be non-atomic and potentially disruptive.)
Yes, great work, thanks! As I hope you saw, I put out a call on the libraries list in case anyone has something to add. I'll give that a few days to percolate. Unless some serious question comes to light, I'll merge in about a week. |
@treeowl For whatever should be merged:
|
I've quite forgotten about it. |
@treeowl I had also forgotten. I see that at the time I was ready to merge - except it wasn't clear whether I'm supposed to merge just this, just #10 , or both. You asked @simonmar to give a look at #10 and he seemed to be fine with it. So @tchoutri thanks for the ping! I am ready to merge as soon as someone tells me what to merge - this, #10, or both. We now need to move from travis CI to github actions - i'll do that now. |
I think #10 is good to go. With this one, there's still a question in my mind about whether you want to force values whose |
Co-authored-by: ygale <[email protected]>
OK thanks. My opinion is - don't worry about do-nothing But I'm not a decision maker here. I'm just trying to help out by providing a home for an important library that became an orphan at some point. So I'll defer to you. I'll merge #10 . Let me know if and when I should merge this PR. |
@treeowl If you do take my dubious advice and not worry about do-nothing Before release I am waiting for the final decision about this PR. Or a decision to push it off for now. In any case, let's not fall back into a collective stupor again. Please ping me and wake me up within the next few days! Thanks. |
Remove conditional import of `tryReadMVar` for re-export in non-ancient versions. Instead, we are making the export itself conditional, only for the ancient versions where it is needed. That is a slightly less invasive change.
I will review to see if I've changed the strictness in do-nothing rnf. If so, I'll probably undo that for PVP reasons and we can discuss it further later. |
@treeowl hmm this got stuck again. Do I merge this PR as is? Are we making further changes? Are we abandoning it altogether? Please let's decide and finally get this off the table. Thanks! |
Fixes #2
Fixes #3
Fixes #4
Fixes #5
Fixes #6
Fixes #8