Much better!

I think there should also be one or two tests that don't rely on all the details of this particular case. To begin with, there should surely be a simple example of defining the same recursive function twice with different names.

In D4967#137105, @simonmar wrote:

@Phyx should have a look at this. Also, can you run a benchmark? It's easy to make things theoretically faster but practically slower.

I believe there were people working on porting the IO manager to Windows which would presumably make this redundant, but I'm not sure what the status of those efforts is, perhaps @Phyx knows.

@simonmar, I see what you likely mean about the PSQ implementation now: it has the advantage of being specialized to fixed-width priorities. We could probably find/create something similarly appropriate for the Windows module, but I don't have the time to invest in it right now (note: we'd need something with a merge operation to maintain the current behavior; the PSQs here don't currently have one).

Another fixup

Fix -Wall

Remove module import cycle

Fix module name; add documentation.

In D4967#136826, @simonmar wrote:

It doesn't look like this compiles.

Did you consider using GHC.Event.PSQ instead?

• Restore original semantics

I've restored the original semantics for now. We can revisit that later. The latest version is less aggressive, but at least shouldn't have the earlier regression.

I just realized my last change makes a slight semantic difference, but one I expect is probably tolerable. With the old version,

@simonmar, I realized last night that my nice, simple definition of atomicModifyIORef' could lead to extra thunks in certain cases. I believe I've cleaned that up, but I'd appreciate a second look.

• Tweak the definition of atomicModifyIORef'

In D4884#136463, @simonmar wrote:

For atomicSwapMutVar# it looks like we don't have the right primitive in Cmm yet, namely %xchg. I suppose we could defer that for a separate diff?

What testing do we have for atomicModifyIORef and friends? It would be good to have some tests to check the strictness properties at least.

Fix atomicModifyIORef' strictness

@simonmar, @fryguybob, I've implemented atomicModifyMutVar2# and atomicModifyMutVar_#, but I could use some help with atomicSwapMutVar#. Could one of you give me a hand?

Implement atomicModifyMutVar_#, change some names, and remove the
unaccepted additions to Data.IORef.

• Back out lazy ST changes

• Back out lazy ST changes

Oh, bleh! I just realized my lazy version is still wrong. It's too strict sometimes, and I think it can also have trouble from the putMVar not actually happening. What a mess....

In D4948#136310, @simonmar wrote:

Can you add a test please?

• Fix lazy ST; add tests

I'm getting very inconsistent benchmark results. This does much better and much worse than the current implementation depending on the situation, and I don't yet understand why.

• Put back one MVar

• Put back one MVar

While the code no longer uses uninterruptibleMask, it remains possible for contention to lead to a thread being unkillable for a time. It's always possible for a thread to be descheduled between performing its atomicModifyIORef and forcing the result. In that case, it will leave a thunk in the IORef. In particular, signalQSemN can leave a good chunk of reversing work behind (it can also leave dead-thread-skipping work behind, but I think it's probably reasonable to assume there won't be too much of that). I don't think there's any way to give an absolute guarantee that no thread will be unkillable for long, but if that's an important goal, I think we can get pretty close by using a non-amortized queue. At first, I thought maybe we could just force the IORef contents at the beginning of each waitQSemN (before masking) but that would violate the ordering guarantee: threads attempting waitQSemN while an expensive thunk was being evaluated would be reordered arbitrarily. I don't know if the benefits of a fancier queue would be worth the price.

• Use an IORef for QSem

• Use an IORef for QSem

@simonmar, I just made a much bigger change that you're likely to find interesting. Note that uninterruptibleMask is gone now.

• Use an IORef for QSem

A test does sound like a good idea.

• Return old value too

• Return old value too

• Return old value too

• Return old value too

• Return old value too

• Return old value too

• Take fryguybob's suggestion
• Rename

• Implement fryguybob's suggestion

• atomicReplaceIORef

Fix mistakes

Fix Windows mistake

Fix mistake

Add needed extension

Tweak

Remove now-unnecessary seq

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

In D4884#134680, @simonmar wrote:

The commit title and summary don't really tell me what this does - can you say what the goal is here? Is there a ticket?

Update stat (too good)

• Fix up Windows-specific code

Remove unused binding

Update docs; clean up

Yes, that. But do you really need such fancy logic in the test? Wouldn't something much simpler (like a type error) do the trick? Then you wouldn't have to deal with -ddump-parsed-ast.

Can you add a test case?

Although it's unfortunate to change benchmark source in general, I think it's pretty clearly justified this time.

• Add output

I believe this change needs a formal libraries list proposal.

Yup!

Yeah, that sounds reasonable. Not ideal, perhaps, but there's no obvious and easy way to make GHCi fail this without really reworking the test.