[atomic-primops] Change in `fetchAddIntArray#` in GHC 7.10 about haskell-lockfree HOT 10 CLOSED

But I do think we should go ahead and bump the major version and change the interface for this Counter lib to return the BEFORE value (and support that behavior across the last 3 GHC versions), what do you think?

Sounds good to me. Sorry about the breakage.

from haskell-lockfree.

Is that how all of the new fetch*IntArray# functions behave? If so that's good, I think, since e.g. for the fetchAnd, fetchOr, etc. we can determine the value after if the value before the atomic op, but not vice versa. And so maybe the naming and behavior of what we have in atomic-primops should be reconsidered? (obviously we can't just change the functionality of fetchAddByteArray; that would need a rename).

(sorry to be lazy here; I just got 7.10 installed and have to run; didn't want to forget to comment on this issue)

from haskell-lockfree.

Sorry, I didn't read your comment properly, @rrnewton.

But I do think we should go ahead and bump the major version and change the interface for this Counter lib to return the BEFORE value (and support that behavior across the last 3 GHC versions), what do you think?

Literally just yesterday I made a commit in my lib that got rid of the upper bounds on atomic-primops... ugh. Can we think of any other names that would be equally good that you can use going forward (with this new behavior), and just deprecate incrCounter and fetchAddByteArray (keeping the old behavior there)?

I've been very paranoid about my use of atomic-primops and my tests can probably detect whatever wonky behavior changes you can throw at me, but I doubt most others have been quite so paranoid. It's not hard to imagine this causing subtle off-by-one errors, and those errors leading to segfaults. And with so many people split on the merits of upper bounds, I just think this is a bad, bad idea.

from haskell-lockfree.

Can we think of any other names that would be equally good that you can use going forward (with this new behavior), and just deprecate incrCounter and fetchAddByteArray (keeping the old behavior there)?

I propose the following; let me know your thoughts:

1. Just using the names from GHC.Prim for the fetch* functions.

• So fetchAddByteArrayInt would be deprecated,
• with CPP conditionals to keep the behavior the same through 7.10.
• While a new wrapper named fetchAddIntArray would take its place with the new behavior (again conditionally bringing this NEW behavior to GHC <7.10). Likewise for the new functions (#43). This would avoid breakage, and I think make more sense (e.g. that's what the primitive package seems to do).

2. Do one of the following:
   • Remove all the Data.Atomic.Counter* modules; it's trivial to implement the functionality using primitive, the package is called atomic-primops, it's not clear to me why those alternate submodules are there, means less code to maintain, etc.
   • Rewrite incrCounter with CPP conditionals to keep the old behavior going forward.
3. Deprecate the new 0.6.1.1, or adjust the base constraint back to <=4.8

from haskell-lockfree.

This sounds like a good plan to me.

My inclination would be to get rid of the Data.Atomic.Counter.* alternatives, which were just there for benchmarking, but leave the Data.Atomic.Counter for now. I agree, it should have gone in a separate package. (Moreover, there should be a proper package of several different counter variants, including scalable counters like SNZI, which we've implemented but not factored out of LVish.) But it also does little harm, and as you say we can prevent breakage and retain the old behavior of incrCounter.

In fact, we could do like C++'s x++/++x if we liked and have both incrCounter and counterIncr to provide both options.

from haskell-lockfree.

@rrnewton sorry for the delay, I'm just starting on some of this work this evening. I see you moved the counter alternatives to other-modules; is it okay to remove them and start simplifying the test suite, etc? I'm also planning on adding a benchmark suite, so I can just do a git mv of those modules there if we only want them as a baseline for benchmarking.

I thought I might do some work that builds on @7315deed31 on master directly, and then submit pull requests for work on #41 and #43. Does that sound good?

from haskell-lockfree.

Sounds good to me, including removing/moving the other counters. Yes, they were mainly there for benchmarking.

It's mentioned somewhere that this separate ./testing/ director was a work-around for cabal/GHC problems (maybe it was haskell/cabal#1284). My takeaway was that foreign primops were not actually used by very many cabal packages and we were stressing corner cases. With new versions of cabal it might work to fold the test-suite back into a normal cabal test suite, and to likewise create a normal cabal benchmark suite.

By the way, we love criterion, but when doing criterion for parallel stuff we tend to do a bit of a hack to make it work well. (You may use the same or a different hack in your code, I'd be interested to hear.)

Basically, if we let criterion run a batch of iterations with the parallelism (forkIO, runPar, whatever), launched from inside each iteration. Then we pay that overhead N times, and it dominates running time. So we want parallel threads to be launched once, and then the N iterations to happen in a sequential loop inside each thread. Fortunately, Criterion exposes the Benchmarkable type constructor, so we get to construct our own Int64 -> IO () functions to launch a batch of iterations.

We employ this technique in the simple MicroBench.hs file that's already in there. (As well as in other libraries.)

from haskell-lockfree.

I think at this point (af637d7) if you wanted to do a release we can have a version that works fully with 7.10, while we work on #43. Also @rrnewton can you edit the metadata for 0.6.1.1 so that it reverts the constraint to base (>=4.6.0.0 && <4.8.0.0)?

from haskell-lockfree.

Sorry, by "edit the metadata" I mean edit the cabal file for that version directly on hackage via the "edit package information" button. That way we don't have a version with different counter/fetchAdd behavior that needs to be blacklisted in other packages.

from haskell-lockfree.

Ok, I adjusted the version constraint and checked that off in the checklist above. I released the new version as "0.7" (following PVP, since it removes modules, even though it doesn't add all the new stuff yet).

I also added you as a maintainer on the hackage package in case I messed anything up or there's anything else you need to do.

from haskell-lockfree.