Unpack and simplify QSem
Needs RevisionPublic

Authored by dfeuer on May 27 2018, 5:14 PM.

Details

Summary
  • Unpack a few pieces of QSem to reduce the amount of allocation per operation.
  • Change the shape of the semaphore value to clarify that there is *either* a resource available *or* a queue of waiting threads.
  • Use tryPutMVar instead of an awkward combination of tryTakeMVar and putMVar.
  • Expand comment on the purposes/effects of the outermost MVar. This particular MVar looks really bad from a contention standpoint; I don't know if there's a better way.
dfeuer created this revision.May 27 2018, 5:14 PM

Could you try a benchmark or two to find out the benefit of this?

libraries/base/Control/Concurrent/QSem.hs
86

This $! is unnecessary

89

$! unnecessary here too

105–108

Nice

tdammers added inline comments.
libraries/base/Control/Concurrent/QSem.hs
50

I'm curious what the reasoning behind changing strictness here is.

dfeuer added inline comments.May 28 2018, 9:09 AM
libraries/base/Control/Concurrent/QSem.hs
50

I haven't really changed any strictness. I've just informed the compiler about strictness that was already there.

86

Yes, I'm aware. I had it there for clarity, but if you think it's better without then I'll yank it.

tdammers accepted this revision.May 30 2018, 10:22 AM
tdammers added inline comments.
libraries/base/Control/Concurrent/QSem.hs
50

Oh, that wasn't obvious to me. But on further reading it seems to be the case.

This revision is now accepted and ready to land.May 30 2018, 10:22 AM

Was there ever any benchmarking of this?

dfeuer added inline comments.Jun 25 2018, 1:08 PM
libraries/base/Control/Concurrent/QSem.hs
105–108

Nicer than I realized, I think. If I'm not mistaken, this lets us replace the outermost MVar with an IORef if we like. In particular, it seems to get rid of the race condition between signaling and self-dequeueing. There's just a bit of unsafePerformIO ugliness to make it work, I believe. I wonder how that will perform.

dfeuer updated this revision to Diff 17089.Jun 25 2018, 2:46 PM
  • 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.

dfeuer marked 3 inline comments as done.Jun 25 2018, 2:51 PM
dfeuer updated this revision to Diff 17090.Jun 25 2018, 3:10 PM
  • Use an IORef for QSem
dfeuer updated this revision to Diff 17093.Jun 25 2018, 5:31 PM
  • Use an IORef for QSem
bgamari requested changes to this revision.Oct 15 2018, 1:03 PM

@dfeuer, what is the status of this? It's not clear that there was ever any performance testing of this done.

This revision now requires changes to proceed.Oct 15 2018, 1:03 PM