Skip to content

Fix a space leak in list traversal #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions indexed-traversable/src/WithIndex.hs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
{-# LANGUAGE GADTs #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE UndecidableInstances #-}
{-# LANGUAGE MagicHash #-}
{-# LANGUAGE ScopedTypeVariables #-}

#if __GLASGOW_HASKELL__ >= 702
{-# LANGUAGE Trustworthy #-}
Expand All @@ -19,7 +21,7 @@ module WithIndex where

import Prelude
(Either (..), Functor (..), Int, Maybe (..), Monad (..), Num (..), error,
flip, id, seq, snd, ($!), ($), (.), zip)
flip, id, seq, snd, ($!), ($), (.))

import Control.Applicative
(Applicative (..), Const (..), ZipList (..), (<$>), liftA2)
Expand All @@ -45,6 +47,7 @@ import Data.Sequence (Seq)
import Data.Traversable (Traversable (..))
import Data.Tree (Tree (..))
import Data.Void (Void)
import GHC.Exts (Int (..), Int#, (+#))

#if __GLASGOW_HASKELL__ >= 702
import GHC.Generics
Expand Down Expand Up @@ -263,9 +266,30 @@ instance FoldableWithIndex Int [] where
go !n (x:xs) = f n x (go (n + 1) xs)
{-# INLINE ifoldr #-}
instance TraversableWithIndex Int [] where
itraverse f = traverse (uncurry' f) . zip [0..]
itraverse f = itraverseListStarting 0 f
{-# INLINE itraverse #-}

-- | Traverse a list with the given starting index. We used to define
-- traversals for @[]@ and 'NonEmpty' using 'Prelude.zip'. Unfortunately, this
-- could sometimes fail to fuse (at least for the @[]@ case), leading to
-- @[0..]@ being allocated as a CAF and walked on each traversal, which is both
-- a space leak and slow. See https://gitlab.haskell.org/ghc/ghc/-/issues/22673
-- Using a manual counter puts a stop to that, and using 'foldr' gives us a
-- chance of fusing with the argument. I didn't see similarly disastrous
-- behavior with 'NonEmpty', but defining its traversal this way produces a
-- rather more readable unfolding that I'm more confident won't go wrong
-- somehow.
--
-- Why do we unbox the counter by hand? GHC /can/ do that itself, but for some
-- reason it only happens with @-O2@, and we use the standard @-O1@.
itraverseListStarting :: forall f a b. Applicative f => Int -> (Int -> a -> f b) -> [a] -> f [b]
itraverseListStarting (I# i0) f = \xs -> foldr go stop xs i0
where
go x r !i = liftA2 (:) (f (I# i) x) (r (i +# 1#))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be possible to do without relying on more unsafe (Safe Haskell wise) modules. I won't import GHC.Exts.

Copy link
Collaborator

@phadej phadej Dec 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And apparently this is one additional instance where -ffull-lazyness is a pessimisation.

I won't accept this kind of patch without a reference to a GHC issue, which IMO this is. The [0..] in itraverse implementation shouldn't be memoized and GHC should have a means to express that, or it should not use -ffull-lazyness by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phadej Please propose an alternative to GHC.Exts that I can use.

Copy link
Collaborator

@phadej phadej Dec 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we unbox the counter by hand? GHC /can/ do that itself, but for some
reason it only happens with @-O2@, and we use the standard @-O1@.

Report a GHC issue. We should know why we write code the way we do. It might be a GHC bug, or it might be an inherent limitation (i.e. GHC cannot do better). We should know the reason.

Copy link
Collaborator

@phadej phadej Dec 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stop :: Int# -> f [b]
stop !_i = pure []
{-# INLINE itraverseListStarting #-}

-- TODO: we could experiment with streaming framework
-- imapListFB f xs = build (\c n -> ifoldr (\i a -> c (f i a)) n xs)

Expand All @@ -292,7 +316,7 @@ instance FoldableWithIndex Int NonEmpty where
{-# INLINE ifoldMap #-}
instance TraversableWithIndex Int NonEmpty where
itraverse f ~(a :| as) =
liftA2 (:|) (f 0 a) (traverse (uncurry' f) (zip [1..] as))
liftA2 (:|) (f 0 a) (itraverseListStarting 1 f as)
{-# INLINE itraverse #-}

-------------------------------------------------------------------------------
Expand Down