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
Replace atomicModifyMutVar# #149
Conversation
35395fd
to
aa2779e
Compare
Replace `atomicModifyMutVar#` with a new, slightly simpler primop whose strictness is easier and more efficient to control. [Rendered view](https://github.com/treeowl/ghc-proposals/blob/atomicModifyMutVar/proposals/0000-atomicModifyMutVar.rst)
aa2779e
to
72b3184
Compare
5682dba
to
d5fee2d
Compare
97ebece
to
909b536
Compare
I just realized we can also return the previous value in the |
2d12051
to
5387ffb
Compare
I'm confused by all the lazy/strict combinations you proposed and each version's usecase, but it seems this is not something we can do with a single primitive, My problem with original
So can your If we have the strict version, we can optimize |
I'm not aiming to solve (2) here. The current design uses three thunks: one for the function application and one to select each of its components. But only two of those thunks are actually necessary. My aim here is to remove the third, which gives us a better version of the |
After reading the cmm code ( We could save the third thunk by directly return the tuple's thunk indeed, |
Seems to me that this proposal is simple and obvious enough that we could also add a fully strict version as @winterland1989 suggested without a separate proposal. But I'm not a direct stakeholder here, just sayin'. |
The proposal asks:
I assume that the "compatibility wrapper" refers to |
Just some of my personal thoughts, from design perspective, the old
Of course your proposed version is even better, but saving one selection thunk which is not on CAS loop path may not reflect on benchmark figures. I'm hesitated adding it mainly for backward compatibility issues. But if everybody think that's not a problem, then a weak +1 from me. On the other hand, i'm strongly +1 in adding a separated |
@winterland1989, I could add as an alternative the option of adding the new primop without removing the old. But I really don't think very many packages both (1) use the primop directly and (2) import it from I definitely think we should have There's an additional intermediate primop in this space that arguably should exist (I mentioned it above in a lousy comment I've since deleted): -- Returns the old value and the new one.
atomicModifyMutVar_# :: MutVar# a -> (a -> a) -> State# s -> (# State# s, a, a #) Whereas |
cd4d098
to
b874747
Compare
b874747
to
3987c74
Compare
@nomeata, while there are still some unresolved questions, discussion appears to have died down without addressing them. I think I'd like to submit this to the committee. |
8a8f69e
to
2ce2517
Compare
@simonmar, I realized the other day that we could additionally return the thunk that we install in the |
@treeowl it's a selector thunk, so once the pair is evaluated the GC will eliminate it, so I don't think there's any benefit to returning it, no. |
@simonmar, the hypothetical reason to want it is to avoid a tiny bit of extra allocation if you want to stash it somewhere else lazily. My general feeling is that virtually all real applications should force the (pair) result of the function anyway, even if they don't force either component, so returning the first component thunk is API clutter and confusion without practical benefit. I just figured I should check that you agree. If you really do want that extra copy, you can get it using the following code at the cost of a bit of extra ephemeral allocation. Note that if you instead just take the first component of the result of calling atomicModifyIORefWithNewThunk
:: IORef a -> (a -> (a, b)) -> IO (a, a, (a, b))
atomicModifyIORefWithNewThunk ref f = do
(old, (new, r)) <- atomicModifyIORef2 ref $ \x ->
let fx = f x
new = fst fx
in (new, (new, fx))
return (old, new, r) |
I see that @simonmar has already commented here -- but what about the relationship of this proposal to the mutable structs one? |
The mutable fields proposal would eventually eliminate the However,
So I wouldn't let mutable fields stand in the way of this small improvement. |
@simonmar, since typical hardware doesn't support multi-word CAS, I imagine primitive atomic operations will be limited to a single field for the foreseeable future. The operations are likely to be fairly similar. |
@treeowl sure. If you want to have a go at adding the atomic primops to the mutable fields proposal that would be great. |
@treeowl would you kindly add high level atomic operations for primitive arrays too? I'm no expert in Cmm, but I think current atomic api for primitive arrays are quite difficult to use, maybe there is a reason why current API are designed this way? |
@winterland1989, I don't know much about CMM either. Fortunately, |
@treeowl regarding the acceptance of this proposal: the changes to |
The proposal has been accepted; the following discussion is mostly of historic interest.
Replace
atomicModifyMutVar#
with a new, slightly simplerprimop whose strictness is easier and more efficient to
control.
Rendered view