Don't allocate in atomicWriteIORef
Needs ReviewPublic

Authored by dfeuer on Jun 22 2018, 2:07 PM.

Details

Summary

atomicWriteIORef was implemented using atomicModifyIORef,
which has to allocate memory. But atomicWriteIORef doesn't need
nearly that much power; a simple CAS loop can do the trick.

Note: The primop alternative is explored in D4887.

dfeuer created this revision.Jun 22 2018, 2:07 PM

Question: might some programs be *relying* on allocation in atomicWriteIORef to yield? If so, should we insert a yield right at the beginning?

dfeuer updated this revision to Diff 17053.Jun 22 2018, 2:20 PM

Remove now-unnecessary seq

dfeuer updated this revision to Diff 17055.Jun 22 2018, 2:59 PM

Add needed extension

dfeuer updated this revision to Diff 17062.Jun 22 2018, 4:29 PM

Fix mistake

dfeuer updated this revision to Diff 17063.Jun 22 2018, 4:30 PM

Fix Windows mistake

dfeuer updated this revision to Diff 17065.Jun 22 2018, 5:35 PM
  • atomicReplaceIORef
dfeuer updated this revision to Diff 17066.Jun 23 2018, 1:47 AM
  • Implement fryguybob's suggestion

A test would be nice.

libraries/base/GHC/IORef.hs
23

let's export casIORef too? It's sometimes useful if you don't want to pull in atomic-primops

65

Needs documentation

67

Is this really the only way to implement atomicModifyIORef? It seems like overkill to have a CAS loop just to get a barrier. Why would we need to loop?

76–80

I'd really like to understand what the danger is here, the comment is a bit vague. Is there a real problem? If so, what's the assumption we're making that would be invalid, how would it be invalidated, and how does lazy fix it? How will we know if it goes wrong?

dfeuer added inline comments.Mon, Jun 25, 6:04 AM
libraries/base/GHC/IORef.hs
67

Good point. We surely need a loop to implement the replacement/swap operation, but shouldn't need it for just writing. But I don't know off the top of my head how to just ask for a barrier.

fryguybob added inline comments.Mon, Jun 25, 7:30 AM
libraries/base/GHC/IORef.hs
67

I think we want atomicWriteIORef to be implemented as XCHG on x86. Depending on what you are doing, atomicWriteIORef or writeIORef may be appropriate.

A test does sound like a good idea.

libraries/base/GHC/IORef.hs
67

Would that be sufficient to implement atomicReplaceIORef (which will be renamed atomicSwapIORef based on a suggestion by "winter")? I'm way outside my comfort zone here; could you advise on implementation details, or perhaps commandeer this differential to do it right?

76–80

I thought we could maybe have trouble if the result was demanded strictly, but now I realize we won't: we're saved by the fact that old is only returned conditionally.

fryguybob added inline comments.Mon, Jun 25, 8:12 AM
libraries/base/GHC/IORef.hs
67

Yes, that should be exactly atomicReplaceIORef. Doing it right is a lot of work :D. For one take on this see: https://dl.acm.org/citation.cfm?id=3018746. Also some relevant earlier discussion here: http://blog.ezyang.com/2014/01/so-you-want-to-add-a-new-concurrency-primitive-to-ghc/

simonmar added inline comments.Mon, Jun 25, 1:11 PM
libraries/base/GHC/IORef.hs
67

maybe just add an xchgMutVar# primitive for now? That would at least be better than what we have.