Add finalization support to folds #2427

harendra-kumar · 2023-07-12T19:08:45Z

This is a revival of #904 .

adithyaov · 2023-07-15T15:04:20Z

core/src/Streamly/Internal/Data/Fold.hs

-takeEndBy_ predicate (Fold fstep finitial fextract) =
-    Fold step finitial fextract
+takeEndBy_ predicate (Fold fstep finitial fextract ffinal) =
+    Fold step finitial fextract ffinal


I don't know if I understand this properly. Why don't we finalize this fold on Done?

Ah, the one who initializes, finalizes.

Only if you initialize it more than once you finalize it.

On Done we need to finalize this fold.

core/src/Streamly/Internal/Data/Fold.hs

adithyaov · 2023-07-16T16:48:12Z

core/src/Streamly/Internal/Data/Fold.hs

-takeEndBy predicate (Fold fstep finitial fextract) =
-    Fold step finitial fextract
+takeEndBy predicate (Fold fstep finitial fextract ffinal) =
+    Fold step finitial fextract ffinal


On Done you need to finalize this fold.

adithyaov · 2023-07-16T18:33:51Z

core/src/Streamly/Internal/Data/Fold/Type.hs

+-- An alternative to using an "extract" funciton is to use "Partial s b" style
+-- partial value so that we always emit the output value and there is no need
+-- to extract. Then extract can be used for cleanup purposes. But in this case
+-- in some cases we may need a "Continue" constructor where an output value is
+-- not available, this was implicit earlier. Also, "b" should be lazy here so
+-- that we do not always compute it even if we do not need it.


I did not understand this very well.

adithyaov · 2023-07-16T18:38:08Z

core/src/Streamly/Internal/Data/Fold/Type.hs

@@ -829,6 +863,9 @@ splitWith func (Fold stepL initialL extractL) (Fold stepR initialR extractR) =
                Partial sR -> extractR sR
                Done rR -> return rR

+    final (SeqFoldR f sR) = finalR sR >>= pure . SeqFoldR f
+    final (SeqFoldL sL) = finalL sL >>= pure . SeqFoldL


If the finalize type was s -> m b. The implementation would've been semantically different here.

In extract we do initialR. Shouldn't we also finalize it there?

adithyaov · 2023-07-16T18:38:48Z

core/src/Streamly/Internal/Data/Fold/Type.hs

    extract (SeqFoldR_ sR) = extractR sR
    extract (SeqFoldL_ _) = do
        res <- initialR
        case res of
            Partial sR -> extractR sR
            Done rR -> return rR

+    final (SeqFoldR_ sR) = finalR sR >>= pure . SeqFoldR_
+    final (SeqFoldL_ sL) = finalL sL >>= pure . SeqFoldL_


In extract we do initialR. Shouldn't we also finalize it there?

adithyaov · 2023-07-17T08:43:49Z

core/src/Streamly/Internal/Data/Fold/Type.hs

@@ -802,8 +831,10 @@ data SeqFoldState sl f sr = SeqFoldL !sl | SeqFoldR !f !sr
 {-# INLINE splitWith #-}
 splitWith :: Monad m =>
    (a -> b -> c) -> Fold m x a -> Fold m x b -> Fold m x c
-splitWith func (Fold stepL initialL extractL) (Fold stepR initialR extractR) =
-    Fold step initial extract
+splitWith func


This should not be a fold

adithyaov · 2023-07-17T08:43:54Z

core/src/Streamly/Internal/Data/Fold/Type.hs

@@ -846,8 +883,8 @@ data SeqFoldState_ sl sr = SeqFoldL_ !sl | SeqFoldR_ !sr
 --
 {-# INLINE split_ #-}
 split_ :: Monad m => Fold m x a -> Fold m x b -> Fold m x b
-split_ (Fold stepL initialL _) (Fold stepR initialR extractR) =
-    Fold step initial extract
+split_ (Fold stepL initialL _ finalL) (Fold stepR initialR extractR finalR) =


This should not be a fold

harendra-kumar · 2023-11-14T11:34:28Z

benchmark/Streamly/Benchmark/FileSystem/Handle/ReadWrite.hs

@@ -83,6 +83,8 @@ copyStream inh outh = S.fold (FH.write outh) (S.unfold FH.reader inh)
 inspect $ hasNoTypeClasses 'copyStream
 inspect $ 'copyStream `hasNoType` ''Step -- S.unfold
 inspect $ 'copyStream `hasNoType` ''IUF.ConcatState -- FH.read/UF.many
+-- XXX Why is this change done?
+-- XXX Is this a fix?


Some inspection tests are failing after the changes, disabled those. We should look at the perf benchmarks instead.

harendra-kumar · 2023-11-14T11:35:02Z

benchmark/Streamly/Benchmark/Unicode/Stream.hs

@@ -285,6 +288,7 @@ _copyStreamUtf8'Fold :: Handle -> Handle -> IO ()
 _copyStreamUtf8'Fold inh outh =
   Stream.fold (Handle.write outh)
     $ Unicode.encodeUtf8
+-- XXX I'm reviewing the files in order. I don't understand this change yet. I might not have reviewed the prerequisite yet!


writeCharUtf8' is a parser now.

harendra-kumar · 2023-11-14T11:56:29Z

core/src/Streamly/Internal/Data/Fold.hs

+-- initialize should give a token and finalize should consume that token. Or the
+-- `Done` should somehow consume that token.
+-- Or can we make a simple assert by keeping a counter? We increment it on
+-- initialize and decrement it on a finalize or `Done`.


There is no such feasible way. The rule is when you are Done or extract you need to finalize any pending state.

harendra-kumar · 2023-11-14T11:59:24Z

core/src/Streamly/Internal/Data/Fold.hs

@@ -1621,6 +1630,7 @@ takeEndBy_ predicate (Fold fstep finitial fextract ffinal) =
    step s a =
        if not (predicate a)
        then fstep s a
+        -- XXX Need to finalize that value here!


harendra-kumar · 2023-11-14T12:00:26Z

core/src/Streamly/Internal/Data/Fold.hs

@@ -1653,6 +1663,7 @@ takeEndBy predicate (Fold fstep finitial fextract ffinal) =
        then return res
        else do
            case res of
+                -- Need to finalize the value here.


harendra-kumar · 2023-11-14T14:13:17Z

core/src/Streamly/Internal/Data/Stream/StreamD/Nesting.hs

@@ -2815,7 +2815,7 @@ splitOnSuffixSeq withSep patArr (Fold fstep initial done final) (Stream step sta
                Stop -> do
                    -- do not issue a blank segment when we end at pattern
                    if (idx == maxIndex) && RB.unsafeEqArray rb rh patArr
-                    then return Stop
+                    then return Stop -- XXX Finalize the fold here


harendra-kumar · 2023-11-14T14:13:24Z

core/src/Streamly/Internal/Data/Stream/StreamD/Nesting.hs

@@ -2847,7 +2847,7 @@ splitOnSuffixSeq withSep patArr (Fold fstep initial done final) (Stream step sta
                Skip s -> go SPEC fs s rh cksum
                Stop ->
                    if RB.unsafeEqArray rb rh patArr
-                    then return Stop
+                    then return Stop -- XXX Finalize the fold here?


harendra-kumar · 2023-11-14T14:22:52Z

core/src/Streamly/Internal/Data/Stream/StreamK.hs

@@ -514,6 +516,7 @@ foldConcat
                    FL.Done b -> return $ Right (b, s)
                    FL.Partial fs1 -> go1 SPEC fs1 s
            Stream.Skip s -> go1 SPEC fs s
+            -- XXX This is being finalized twice


removed this finalization.

harendra-kumar · 2023-11-14T15:12:46Z

src/Streamly/Internal/Data/Fold/SVar.hs

@@ -58,6 +58,8 @@ writeLimited svar winfo = Fold step initial (const (return ())) final

    where

+    -- XXX We never change the state of the fold to "False". What is the point
+    -- og this state?


This is old code, and deprecated, not changing it.

harendra-kumar · 2023-11-14T15:13:21Z

src/Streamly/Internal/Data/Fold/Concurrent/Channel.hs

+    -- XXX Thinking out loud without understanding the nitty gritty.
+    -- Maybe change the result type to (Maybe b)?
+    -- parEval :: MonadAsync m => (Config -> Config) -> Fold m a b -> Fold m a (Maybe b)
+    -- If it's a parallel fold the result might not exist when er want it to.


Not changing the type.

adithyaov · 2023-11-17T20:00:18Z

core/src/Streamly/Internal/Data/Fold/Type.hs

-            case resL of
-                Partial sL -> bimap (Tuple' sL) Right resR
-                Done bL -> Done $ Left bL
+        case resL of


Ah. Was this a bug fix?

adithyaov · 2023-11-17T20:06:49Z

core/src/Streamly/Internal/Data/Fold/Type.hs

            Done b -> initInnerFold (f b)

-    stepc (C stepInner s extractInner) a = do
+    stepc (C stepInner s extractInner fin) a = do


extractInner is not used anywhere

adithyaov · 2023-11-17T20:16:07Z

core/src/Streamly/Internal/Data/Fold/Type.hs

-                        Done bR -> Done bR
+                case rR of
+                    Partial sR1 -> return $ Partial (sL, sR1)
+                    Done bR -> finalL sL >> return (Done bR)


It is hard to catch if this is missed anywhere. We should have some tests to catch this.

adithyaov · 2023-11-17T20:25:41Z

core/src/Streamly/Internal/Data/Fold/Type.hs

-refoldMany (Fold sstep sinitial sextract) (Refold cstep cinject cextract) =
+refoldMany
+    (Fold sstep sinitial sextract _sfinal)
+    -- XXX We will need a "final" in refold as well


We can open an issue.

adithyaov · 2023-11-17T20:59:53Z

src/Streamly/Internal/Data/Fold/Concurrent/Channel.hs

@@ -62,13 +63,33 @@ parEval modifier f =
    --
    -- A polled stream abstraction may be useful, it would consist of normal
    -- events and tick events, latter are guaranteed to arrive.
+    --
+    -- XXX We can use the config to indicate if the fold is a scanning type or


Can we create an issue based on these comments so we can track them?

This was needed especially for concurrent fold combinators. A fold combinator that uses concurrent folds needs to wait for the concurrent folds to finish before it can finish. The finalizing action in folds can deallocate any resources allocated by the "initial" action and also wait for folds that it has initialized. This complicates fold combinators in general. We can potentially introduce a type for non-failing parsers and support finalization only in those. The current use cases can be covered by that. Parsers do not support scanning, which is not required in the use cases where we need finalization (there is no known use case).

harendra-kumar requested a review from adithyaov July 12, 2023 19:08

adithyaov mentioned this pull request Jul 15, 2023

Add a done action in the Fold type #543

Closed

adithyaov reviewed Jul 15, 2023

View reviewed changes

core/src/Streamly/Internal/Data/Fold.hs Outdated Show resolved Hide resolved

adithyaov reviewed Jul 16, 2023

View reviewed changes

adithyaov reviewed Jul 17, 2023

View reviewed changes

harendra-kumar commented Nov 14, 2023

View reviewed changes

harendra-kumar force-pushed the fold-cleanup-action-done branch 2 times, most recently from 04d4336 to 7392655 Compare November 14, 2023 22:28

adithyaov reviewed Nov 17, 2023

View reviewed changes

adithyaov approved these changes Nov 17, 2023

View reviewed changes

harendra-kumar force-pushed the fold-cleanup-action-done branch from 7392655 to 32390c5 Compare November 18, 2023 18:21

harendra-kumar merged commit 32390c5 into master Nov 18, 2023
13 of 17 checks passed

harendra-kumar mentioned this pull request Nov 19, 2023

Add cleanup to folds #904

Closed

4 tasks

harendra-kumar deleted the fold-cleanup-action-done branch November 22, 2023 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add finalization support to folds #2427

Add finalization support to folds #2427

harendra-kumar commented Jul 12, 2023 •

edited

Loading

adithyaov Jul 15, 2023

adithyaov Jul 15, 2023

adithyaov Jul 15, 2023

adithyaov Jul 16, 2023

adithyaov Jul 16, 2023

adithyaov Jul 16, 2023

adithyaov Jul 16, 2023

adithyaov Jul 16, 2023

adithyaov Jul 17, 2023

adithyaov Jul 17, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

harendra-kumar Nov 14, 2023

adithyaov Nov 17, 2023

adithyaov Nov 17, 2023 •

edited

Loading

adithyaov Nov 17, 2023

adithyaov Nov 17, 2023

adithyaov Nov 17, 2023

Add finalization support to folds #2427

Add finalization support to folds #2427

Conversation

harendra-kumar commented Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adithyaov Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harendra-kumar commented Jul 12, 2023 •

edited

Loading

adithyaov Nov 17, 2023 •

edited

Loading