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

Replace atomicModifyMutVar# #149

Merged
merged 10 commits into from Jul 11, 2018

Conversation

8 participants
@treeowl
Copy link
Contributor

commented Jun 21, 2018

The proposal has been accepted; the following discussion is mostly of historic interest.


Replace atomicModifyMutVar# with a new, slightly simpler
primop whose strictness is easier and more efficient to
control.

Rendered view

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch 3 times, most recently from 35395fd to aa2779e Jun 21, 2018

Replace atomicModifyMutVar#
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)

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch from aa2779e to 72b3184 Jun 22, 2018

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch from 5682dba to d5fee2d Jun 22, 2018

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch from 97ebece to 909b536 Jun 22, 2018

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2018

I just realized we can also return the previous value in the IORef almost for free, so we should do that. I'll make the change. That'll mean users won't have to allocate an extra pair just to get the old value back along with the new one and the result.

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch from 2d12051 to 5387ffb Jun 24, 2018

@winterland1989

This comment has been minimized.

Copy link

commented Jun 24, 2018

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 atomicModifyIORef is:

  1. It will always create a thunk and CAS that thunk with old value, this is a good default design because thunk creating is fast, thus we can avoid contention even if the computation is costly, e.g atomicModifyIORef (\ m -> (HashMap.insert m x, ())).

  2. But some computations are even cheaper than a thunk creating almost as cheap as rewriting thunk's payload, for example (+1), const x', etc. We don't care if these computation are retried. So under these scenarios we want a strict applied version of atomicModifyIORef.

  3. The atomicModifyIORef' in base is definitely not OK for usecase above, it just simply forces the thunk after a successful CAS, which provide a shortcut for usecase in point 1.

  4. atomic-primops provide a strict version i want, but it's implemented in haskell rather than cmm, thus it's not clear if it's optimal.

So can your atomicModifyMutVar# solve the problem here? I don't think so since the primop itself is still lazy in applying the modify function (correct me if i'm wrong). Please also add more usecase you have in mind in the motivation section, since i don't understand each version of your proposed functions' usecase.

If we have the strict version, we can optimize atomicWriteIORef a little since we can skip the \_ -> (a, ()) thunk creating.

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2018

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 atomicModifyIORef' in base, as well as more flexibility overall. Adding your (2) might well be valuable, but it's for a different proposal.

@winterland1989

This comment has been minimized.

Copy link

commented Jun 24, 2018

After reading the cmm code (stg_atomicModifyMutVarzh), i realized that the thunk creating is not on the CAS loop path, thus i have edited my comment above.

We could save the third thunk by directly return the tuple's thunk indeed, but i don't think it's necessary to return the old value explicitly, user can return it in the return value b if needed (though doing that is not as efficient as it could be) re-read your proposal and find you do provide it for efficient purpose.

@ygale

This comment has been minimized.

Copy link

commented Jun 26, 2018

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'.

@andrewthad

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

The proposal asks:

Where should the compatibility wrapper live?

I assume that the "compatibility wrapper" refers to atomicModifyMutVar#, which would no longer be a primop. It seems like GHC.Exts is where this should live.

@winterland1989

This comment has been minimized.

Copy link

commented Jun 26, 2018

Just some of my personal thoughts, from design perspective, the old atomicModifyMutVar# is quite elegant on its own:

  1. the type is simple and easy to understand.
  2. the CAS loop path is very short (a bit of shock when i read the code).
  3. general enough to fit all kind of use case (including return the old value).

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 atomicSwapMutVar# as @treeowl proposed on the mailing list here. It can provide a better atomicWriteIORef as well, please add it to this proposal : )

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

@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 GHC.Prim (which they shouldn't be doing anyway). I think exporting the wrapper from the right place (whether that's GHC.Exts, GHC.Base, or somewhere else) should cover most real uses, and the rest can easily be fixed in a backwards compatible way.

I definitely think we should have atomicSwapMutVar# as well (although I don't know how to write it optimally myself).

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 atomicSwapMutVar# needs zero thunks and atomicModifyMutVar2# needs two thunks, atomicModifyMutVar_# needs one thunk. There seem to be a pretty good number of real-life situations that only need atomicModifyMutVar_#.

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch from cd4d098 to b874747 Jun 26, 2018

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch from b874747 to 3987c74 Jul 3, 2018

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

@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.

@treeowl treeowl force-pushed the treeowl:atomicModifyMutVar branch from 8a8f69e to 2ce2517 Jul 3, 2018

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

@simonmar, I realized the other day that we could additionally return the thunk that we install in the MutVar#. Do you think we should? I don't really think it has much practical value.

@simonmar

This comment has been minimized.

Copy link

commented Jul 6, 2018

@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.

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2018

@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 atomicModifyIORef2# with the given function, then you'll end up with two copies of the selector thunk. I still think that vanishingly few people will want this anyway.

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)
@rrnewton

This comment has been minimized.

Copy link

commented Jul 9, 2018

I see that @simonmar has already commented here -- but what about the relationship of this proposal to the mutable structs one?

@simonmar

This comment has been minimized.

Copy link

commented Jul 10, 2018

The mutable fields proposal would eventually eliminate the MutVar# primops and implement IORef using a mutable field, which would make this proposal redundant.

However,

  1. We haven't even considered atomic operations yet in the mutable fields proposal.
  2. This proposal is already implemented and mutable fields still has quite a long way to go.

So I wouldn't let mutable fields stand in the way of this small improvement.

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@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.

@simonmar

This comment has been minimized.

Copy link

commented Jul 10, 2018

@treeowl sure. If you want to have a go at adding the atomic primops to the mutable fields proposal that would be great.

@nomeata nomeata merged commit 4e0fdbb into ghc-proposals:master Jul 11, 2018

@winterland1989

This comment has been minimized.

Copy link

commented Jul 11, 2018

@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?

@treeowl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

@winterland1989, I don't know much about CMM either. Fortunately, atomicModifyMutVar2# and atomicModifyMutVar_# could be written by just deleting code from the old atomicModifyMutVar# implementation. For something totally new, you might be better off finding someone more experienced in this realm. (Indeed, I'll need some help to get atomicSwapMutVar# written.)

@simonmar

This comment has been minimized.

Copy link

commented Jul 12, 2018

@treeowl regarding the acceptance of this proposal: the changes to Data.IORef should be considered by the libraries committee separately, for now we can go ahead with the changes to the primops.

@bravit bravit added the Proposal label Nov 11, 2018

@nomeata nomeata added the Implemented label Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.