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.

Sign up for GitHub

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

Jump to bottom

Implement Data.Parser.ParserD.deintercalate #843

Closed

specdrake wants to merge 1 commit into composewell:master from specdrake:deintercalate

Collaborator

specdrake commented Dec 18, 2020

No description provided.

specdrake marked this pull request as ready for review

December 19, 2020 19:53

specdrake requested a review from adithyaov

January 2, 2021 10:38

Member

adithyaov commented Jan 2, 2021

@specdrake Can you resolve the conflicts? I'll review it after.

specdrake force-pushed the deintercalate branch from e0b529c to 4b83436 Compare

January 2, 2021 14:09


          implement deintercalate, add tests and benchmarks

377013b

specdrake force-pushed the deintercalate branch from 4b83436 to 377013b Compare

January 10, 2021 17:31

Collaborator Author

specdrake commented Jan 10, 2021

@specdrake Can you resolve the conflicts? I'll review it after.

Can you take a look at this now?

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser.hs

Comment on lines +782 to +788

+              deintercalate fld1 prsr1 fld2 prsr2 =
+                  K.toParserK $
+                      D.deintercalate
+                      fld1
+                      (K.fromParserK prsr1)
+                      fld2
+                      (K.fromParserK prsr2)

Member

adithyaov Feb 10, 2021

Suggested change

      
            deintercalate fld1 prsr1 fld2 prsr2 =
          
                K.toParserK $
          
                    D.deintercalate
          
                    fld1
          
                    (K.fromParserK prsr1)
          
                    fld2
          
                    (K.fromParserK prsr2)
          
            deintercalate f1 p1 f2 p2 =
          
                K.toParserK $ D.deintercalate f1 (K.fromParserK p1) f2 (K.fromParserK p2)

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                      pinit1 <- pinitial1
+                      finit2 <- finitial2
+                      pinit2 <- pinitial2
+                      return (finit1, pinit1, finit2, pinit2, Parser1, 0::Int)

Member

adithyaov Feb 10, 2021

Depending on the benchmarks, you might want to use a strict state, Tuple6'

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                      pinit2 <- pinitial2
+                      return (finit1, pinit1, finit2, pinit2, Parser1, 0::Int)
+                  step (fs1, ps1, fs2, ps2, currentParser, numBuffered) a =

Member

adithyaov Feb 10, 2021 •

edited

Instead of currentParser you can probaly simplify this if you use Bool. Something like isParser1. Your call. You can try both and check the benchmarks. I suspect they won't differ much though.

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                              st <- pstep1 ps1 a
+                              case st of
+                                  Partial n ps1new ->
+                                      return $ Partial n (fs1, ps1new, fs2, ps2,

Member

adithyaov Feb 10, 2021

Put the tuple on the same line?

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

Comment on lines +689 to +693

+                                      let
+                                          newNumBuffered =
+                                              if n==0
+                                              then numBuffered + 1
+                                              else numBuffered - n

Member

adithyaov Feb 10, 2021

If numBuffered + 1 - n < 0 there is some flaw in the logic. You should use asserts instead.
Also, shouldn't it be numBuffered + 1 - n instead of numBuffered - n?

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                                              if n==0
+                                              then numBuffered + 1
+                                              else numBuffered - n
+                                      return $ Continue n (fs1, ps1new, fs2, ps2,

Member

adithyaov Feb 10, 2021

Put the tuple on the same line?

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                                  Done n result -> do
+                                      res1 <- fstep1 fs1 result
+                                      pinit1 <- pinitial1
+                                      pinit2 <- pinitial2

Member

adithyaov Feb 10, 2021

I think there are multiple unrequired initializations here.
Looks like ps2 is pinit2
If you don't need both the states at once, you can probably encode the parser state in the constructors themselves.

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                                                  Parser2, numBuffered)
+                                          FL.Done result1 -> do
+                                              result2 <- fextract2 fs2
+                                              return $ Done numBuffered (result1, result2)

Member

adithyaov Feb 10, 2021

We should just backtrack n elements here? Why numBuffered elements?

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                                      -- input definitively rejected by the first parser
+                                      pinit1 <- pinitial1
+                                      pinit2 <- pinitial2
+                                      return $ Partial numBuffered (fs1, pinit1, fs2, pinit2,

Member

adithyaov Feb 10, 2021 •

edited

You should backtrack numBuffered + 1 elements here instead of numBuffered?
And restart the buffer count from 0 instead of numBuffered - 1?

adithyaov reviewed

View reviewed changes

src/Streamly/Internal/Data/Parser/ParserD.hs

+                                          Parser2, numBuffered-1)
+                                      -- return $ Done numBuffered (result1, result2)
+                          Parser2 -> do

Member

adithyaov Feb 10, 2021

Same comments made above apply to this branch.

adithyaov reviewed

View reviewed changes

benchmark/Streamly/Benchmark/Data/Parser.hs

@@ @@ -59,6 +59,11 @@ benchIOSink value name f = @@
               -------------------------------------------------------------------------------
               -- Parsers
               -------------------------------------------------------------------------------
+              {-# INLINE deintercalate #-}
+              deintercalate :: MonadCatch m => Int -> SerialT m Int -> m ((), ())

Member

adithyaov Feb 10, 2021

Need benchmarks for ParserD as well.

adithyaov reviewed

View reviewed changes

test/Streamly/Test/Data/Parser/ParserD.hs

+              --                 Left _ -> property False
+              deintercalate :: Property
+              deintercalate =

Member

adithyaov Feb 10, 2021

This is being tested in Parser as well. I guess you can skip testing this here.

adithyaov requested changes

View reviewed changes

Member

adithyaov left a comment •

edited

Please address the comments. I see some correctness issues but the tests give no failure.
Either I've misunderstood, if so, please revet back by responding to the comments.
Else we probably need more elaborarte tests hitting all the branches.

I see some indentation issues as well. That's not a big deal though.
I'll reindent this in the end. Diffing that with this will probably makes thing clear w.r.t indentation.

Collaborator Author

specdrake commented Feb 10, 2021

Please address the comments. I see some correctness issues but the tests give no failure.
Either I've misunderstood, if so, please revet back by responding to the comments.
Else we probably need more elaborarte tests hitting all the branches.

I see some indentation issues as well. That's not a big deal though.
I'll reindent this in the end. Diffing that with this will probably makes thing clear w.r.t indentation.

I'll try to do this by today.

Collaborator Author

specdrake commented Feb 17, 2021 •

edited

Please address the comments. I see some correctness issues but the tests give no failure.
Either I've misunderstood, if so, please revet back by responding to the comments.
Else we probably need more elaborarte tests hitting all the branches.

I see some indentation issues as well. That's not a big deal though.
I'll reindent this in the end. Diffing that with this will probably makes thing clear w.r.t indentation.

I had made the changes but I am unable to benchmarks and tests because of the following errors:

    Module
    ‘Streamly.Internal.Data.Time.Clock’
    does not export
    ‘asyncClock’
   |
94 |     (Clock(Monotonic), asyncClock, readClock)
   |                        ^^^^^^^^^^

src/Streamly/Internal/Data/Stream/StreamD/Generate.hs:94:36: error:
    Module
    ‘Streamly.Internal.Data.Time.Clock’
    does not export
    ‘readClock’
   |
94 |     (Clock(Monotonic), asyncClock, readClock)

I have tried using cabal clean as suggested but it also removes chart executable and I have to run
nix-shell --argstr c2nix "--flag dev" --run "cabal build chart --flag dev" but running benchmarks/tests again shows the above error.

specdrake mentioned this pull request

Add a hack to find "chart" executable #950

Merged

Member

harendra-kumar commented May 6, 2022

Superseded by #1600

harendra-kumar closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment