Fix a space leak in list traversal #27
Conversation
| 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#)) |
There was a problem hiding this comment.
This should be possible to do without relying on more unsafe (Safe Haskell wise) modules. I won't import GHC.Exts.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@phadej Please propose an alternative to GHC.Exts that I can use.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
https://gitlab.haskell.org/ghc/ghc/-/issues/22674 for Int# stuff.
EDIT: which was duplicate of https://gitlab.haskell.org/ghc/ghc/-/issues/17504
Previously, we used `zip` to define `itraverse` for lists. This led to two problems: 1. Because the zip fused with the index generator, it could *not* fuse with the argument. 2. I ran into situations where the zip *didn't* fuse with the index generator, so my code ended up actually building *and saving* `[0..]` as a CAF. That's a nasty space leak, as well as slow. Writing `itraverse` for lists using `foldr` directly seems to clear up these issues. Unboxing the counter manually should prevent `Int` boxes from being allocated when the passed function doesn't need them.
There was a problem hiding this comment.
To summarize:
-
I'm not a fan of needing to unbox
Intmanually. I'd like to understand what makes-O2do it for us. -
FunctorWithIndexandFoldableWithIndexinstances are not usingzip [0..]pattern, and it makes sense to be uniform. Theimapandifoldrare not written usingfoldr, so either:- They should be rewritten with
foldras well - Or
itraversecan be written using manual recursion too (and probably GHC will be smart enough to unboxIntthen). This is "worse" because list fusion won't fire, but I actually don't care, as list fusion is unreliably anyway. (And e.g. RULES dance aroundGHC.Base.mapis too complicated).
So I lean towards the latter option: writing
itraversefor lists with explicit recursion.The
itraverseListStartinghelper is good, as it's useful inNonEmptyinstance as well. - They should be rewritten with
Previously, we used
zipto defineitraversefor lists. This led to two problems:Because the zip fused with the index generator, it could not fuse with the argument.
I ran into situations where the zip didn't fuse with the index generator, so my code ended up actually building and saving
[0..]as a CAF. That's a nasty space leak, as well as slow.Writing
itraversefor lists usingfoldrdirectly seems to clear up these issues. Unboxing the counter manually should preventIntboxes from being allocated when the passed function doesn't need them.