Skip to content
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

*> leaks space where >> doesn't - so should the *> implementation be optimized? #496

Closed
alaendle opened this issue Oct 5, 2022 · 1 comment · Fixed by #497
Closed

*> leaks space where >> doesn't - so should the *> implementation be optimized? #496

alaendle opened this issue Oct 5, 2022 · 1 comment · Fixed by #497

Comments

@alaendle
Copy link
Contributor

alaendle commented Oct 5, 2022

Basically an implementation like the following leaks space:

source :: Monad m => [Int] -> ConduitT i Int m ()
source s = case s of
  [] -> pure ()
  n : ns -> yield n *> source ns

Switching to >> avoids the problem. That makes me believe that the implementation of *>could/should be optimized.

To be honest I don't fully understand what causes the different behavior (my assumption is that the applicative implementation isn't aware of the container and so the monad implementation takes shortcuts).

A (maybe dirty) hack to solve this problem is to just implement *> with >> by adding

    (*>) = (>>)
    {-# INLINE (*>) #-}

to

instance Applicative (ConduitT i o m) where
pure x = ConduitT ($ x)
{-# INLINE pure #-}
(<*>) = ap
{-# INLINE (<*>) #-}

(See https://stackoverflow.com/questions/73460640/haskell-based-file-streaming-causes-memory-leak for the original problem)

Please let me know if I was unclear or if I could provide further information. Also I'm willing to provide a PR (but need some guidance if a more sophisticated solution is required).

@snoyberg
Copy link
Owner

snoyberg commented Oct 6, 2022

That description makes sense, and it's the kind of thing that pops up occasionally, especially due to the (now ancient) AMP (Application/Monad Proposal). Your proposed "dirty" hack is actually a perfect approach. Would you be up for opening a PR to make the change?

alaendle added a commit to alaendle/conduit that referenced this issue Oct 6, 2022
So just implemented *> in terms of >>. Resolves snoyberg#496.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants