stm icon indicating copy to clipboard operation
stm copied to clipboard

stateTVar isn't strict

Open KingoftheHomeless opened this issue 5 years ago • 4 comments

From Control.Concurrent.STM.TVar:

stateTVar :: TVar s -> (s -> (a, s)) -> STM a
stateTVar var f = do
   s <- readTVar var
   let (a, s') = f s -- since we destructure this, we are strict in f
   writeTVar var s'
   return a

The comment is incorrect, since let-bindings pattern-match lazily. In order to make this strict, a bang pattern is needed:

let !(a, s') = f s -- since we destructure this, we are strict in f

KingoftheHomeless avatar Jul 11 '20 20:07 KingoftheHomeless

Test-case:

main :: IO ()
main = do
    v <- newTVarIO (7 :: Int)
    x <- atomically $ do
        stateTVar v (error "strict in f")
        writeTVar v 8
        readTVar v
    print x

If we add bang pattern to stateTVar then the code crashes.

I think we should probably add stateTVar' as an equivalent of modifyTVar'. But it's so simple to implement that perhaps it shouldn't even live in the library and we should only fix the incorrect comment.

Fuuzetsu avatar Jul 12 '20 22:07 Fuuzetsu

I would argue that the version of stateTVar that is strict in the tuple would be much more useful than the one that isn't, since it allows you to control what components of the state and result are strict by seq-ing parts to the tuple, while the lazy version doesn't allow you to control strictness at all. In fact, the strict version is more general than the lazy one, since stateTVarLazy f = stateTVarStrict (\s -> let ~(a, s') = f s in (a, s')).

Edit: Actually, they're equally general. stateTVarLazy can be used to implement stateTVarStrict like this:

data Box a = Box a

stateTVarStrict f = do
  Box a <- stateTVarLazy $ \s -> let !(a, s') = f a in (Box a, s')
  return a

But that's inefficient and much more clumsy.

I'm reminded of discussion relating to atomicModifyIORef, and I think the consensus is that it's problematically lazy exactly because of these reasons, although I can't find any example discussion at the moment.

On an unrelated note, perhaps the elements of the tuple should be swapped to mirror atomicModifyIORef?

KingoftheHomeless avatar Jul 13 '20 12:07 KingoftheHomeless

Sorry; I wrote the original code in #14 and was less familiar with strictness then, I assumed it was fine as it passed review. FWIW, I did intend the strict version - the docstring explicitly says it's similar to modifyTVar' the strict form. The code is fairly young so I'd guess not many people are using it yet, and that even less people are depending on the lazy behaviour. So perhaps it's worth just adding the bang in a PR, without defining a second function.

infinity0 avatar Jul 14 '20 23:07 infinity0

On an unrelated note, perhaps the elements of the tuple should be swapped to mirror atomicModifyIORef?

The intention was to mirror state as in MonadState, StateT, etc.

I speculate that the order in atomicModifyIORef comes from an older convention which was swapped, which you still see in the internal definition of IO/ST and GHC internal code.

infinity0 avatar Jul 14 '20 23:07 infinity0