Replace Parsec with Megaparsec (see #289) #366

Merged
merged 7 commits into from Jul 29, 2016

Projects

None yet

3 participants

@cocreature
Collaborator

This builds upon PR #289 by @rasendubi.

I rebased it on master, changed quite a lot of stuff either because it doesn’t make sense with master or because I preferred it that way. All tests are passing so hopefully it doesn’t break anything.

I haven’t done any performance comparisons and sadly I probably won’t have time for that during the next few weeks, so if you want to do that before merging it, it would be great if you could do the measurements yourself.

@simonmichael
Owner
simonmichael commented Jul 10, 2016 edited

Great, thanks!

Is the addition of evalState just cosmetic, or an algorithmic change ?

Shouldn't we avoid reverting the recent renaming of parseWithCtx to parseWithState ?

@simonmichael
Owner

PS as you probably saw, travis reports some doctests failing due to the new error messages. If standard test running procedure is not running doctests, we might need to fix that.

@simonmichael
Owner
simonmichael commented Jul 10, 2016 edited

PPS it would be nice to drop the parsec dependency. I would be happy to switch to a new CSV lib to facilitate that, a new one using megaparsec for good error messages was announced recently. But perhaps this is out of scope.

@simonmichael
Owner

cassava-megaparsec, I'm thinking of. That would require megaparsec 5, which is not yet in stackage LTS, and also pulls in attoparsec.

@cocreature
Collaborator

Is the addition of evalState just cosmetic, or an algorithmic change ?

That’s just cosmetic as megaparsec doesn’t provide a state itself. I’m fine with removing it if you prefer that. That was in the original PR so I just left it in.

Shouldn't we avoid reverting the recent renaming of parseWithCtx to parseWithState ?

Sorry that was caused by the rebase, I’ll push a fix for that in a few minutes.

cassava-megaparsec, I'm thinking of. That would require megaparsec 5, which is not yet in stackage LTS, and also pulls in attoparsec.

Switching to a new CSV lib seems like a separate issue, so I would prefer doing that in a separate PR (this one is large enough) but if you prefer I can also do it here.

PS as you probably saw, travis reports some doctests failing due to the new error messages. If standard test running procedure is not running doctests, we might need to fix that.

Hehe, no your test setup is fine, I was simply only looking at the last line and saw that all of the hunit tests passed and didn’t scroll up. Fix coming in a few minutes.

@cocreature
Collaborator

Alright travis is happy now.

@simonmichael
Owner

I'm a little unclear what's happening with State.. is it added because megaparsec doesn't have parse state built in, unlike parsec ? If so isn't it required ?

@cocreature
Collaborator

I'm a little unclear what's happening with State.. is it added because megaparsec doesn't have parse state built in, unlike parsec ? If so isn't it required ?

Compare the megaparsec state and the parsec state state types, the difference is that parsec has a field for a custom user state that megaparsec doesn’t have. In most cases this state is unnecessary (which is presumably why megaparsec removed it). Where it is necessary (hledger uses it quite heavily for the user state) you need to throw ParsecT ontop of StateT which is what I did (and I added some MonadState polymorphism).

@simonmichael
Owner

It does seem a little slower than parsec right now. Times in seconds:

+--------------------------------------++----------------+--------------------+
|                                      || hledger.parsec | hledger.megaparsec |
+======================================++================+====================+
| stats -f data/100x100x10.journal     ||           0.04 |               0.05 |
| stats -f data/1000x1000x10.journal   ||           0.22 |               0.27 |
| stats -f data/10000x1000x10.journal  ||           1.91 |               2.59 |
| stats -f data/100000x1000x10.journal ||          20.86 |              26.70 |
+--------------------------------------++----------------+--------------------+

Memory:

hledger.parsec:

   3,425,436,192 bytes allocated in the heap
     211,899,640 bytes copied during GC
      37,144,528 bytes maximum residency (10 sample(s))
       2,517,160 bytes maximum slop
              83 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0      6651 colls,     0 par    0.309s   0.328s     0.0000s    0.0011s
  Gen  1        10 colls,     0 par    0.140s   0.258s     0.0258s    0.0886s

  TASKS: 4 (1 bound, 3 peak workers (3 total), using -N1)

  SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.000s  (  0.009s elapsed)
  MUT     time    1.399s  (  1.474s elapsed)
  GC      time    0.449s  (  0.586s elapsed)
  EXIT    time    0.001s  (  0.008s elapsed)
  Total   time    1.852s  (  2.077s elapsed)

  Alloc rate    2,447,992,110 bytes per MUT second

  Productivity  75.7% of total user, 67.5% of total elapsed

hledger.megaparsec:

   3,335,884,936 bytes allocated in the heap
     246,597,144 bytes copied during GC
      34,494,224 bytes maximum residency (11 sample(s))
       2,491,736 bytes maximum slop
              99 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0      6426 colls,     0 par    0.384s   0.430s     0.0001s    0.0021s
  Gen  1        11 colls,     0 par    0.205s   0.259s     0.0236s    0.0781s

  TASKS: 4 (1 bound, 3 peak workers (3 total), using -N1)

  SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.000s  (  0.001s elapsed)
  MUT     time    2.335s  (  2.568s elapsed)
  GC      time    0.589s  (  0.689s elapsed)
  EXIT    time    0.000s  (  0.009s elapsed)
  Total   time    2.926s  (  3.267s elapsed)

  Alloc rate    1,428,489,121 bytes per MUT second

  Productivity  79.9% of total user, 71.5% of total elapsed
@cocreature
Collaborator

It does seem a little slower than parsec right now.

That’s annoying. My understanding is that megaparsec is supposed to be faster than parsec in most cases. I’ll try to take a look, but I’m a bit short on time atm. If we can’t figure out what’s causing the slowdown it might be worth opening an issue to ask for help (pinging @mrkkrp to see if he knows anything about this).

Some things to investigate are

  • Does megaparsec 5.0 improve the performance? The changelog doesn’t seem to mention performance improvements but it’s probably still worth a try.
  • Does transformers 5.2 improve the performance? The biggest change we had to make was the additional state transformer so maybe all the inlining added to transformers helps?
  • This is more of an oversight on my part, we should probably use a strict state monad even if it doesn’t turn out to improve the performance.
@mrkkrp
mrkkrp commented Jul 11, 2016 edited

Megaparsec 5 is faster, yes, use it if you can, although my benchmarks do not use monadic stacks. In particular early versions of Megaparsec have a bit slow imprementation of manyTill and one more combinator (I don't remember which right now). If you run your profiling with Megaparsec 5 (maybe also with strict state monad), you should get better results.


I'll add performance improvements to change log.

@cocreature
Collaborator

Thanks for the quick answer @mrkkrp and keep up the good work!

@cocreature
Collaborator

Alright, building againts nightly-2016-07-10 to get an up2date version of megaparsec and transformers and updating the code for megparsec 5.0 as well as using strict state, I have

read bench/10000x1000x10.journal        [4.52s]
print                                   [4.21s]
register                                [5.53s]
balance                                 [0.46s]
stats                                   [0.48s]
Total: 15.20s

for the megaparsec version and

read bench/10000x1000x10.journal        [4.80s]
print                                   [4.25s]
register                                [5.56s]
balance                                 [0.47s]
stats                                   [0.44s]
Total: 15.51s

(my cpu frequency scaling seems to be broken so the number are probably terrible so ignore the absolute value).

@simonmichael So the question is are you willing to require megaparsec 5? Maintaining compatibilty to older versions will be a giant mess. The only downside is that the master branch will require a stackage nightly (or you add it manually to the stack.yaml, but that seems like a worse idea). Since we need to update to megaparsec 5 at some point it seems like a good idea to do it as early as possible. If you are okay with that I’ll push the necessary updates.

@mrkkrp
mrkkrp commented Jul 11, 2016

@cocreature, FYI, you can make it faster if you use concrete types like Parser from module corresponding to input data (e.g. Text.Megaparsec.Text, Parser = Parsec Dec Text), you can do it with monad stack too, if you define your own Parser type synonym. Is there any reason to keep parser abstract with respect to input data?

@cocreature
Collaborator

@mrkkrp I guess not, I was kinda hoping that GHC would optimize this away for me. I just left them polymorphic because I like parametricity :)

@mrkkrp
mrkkrp commented Jul 11, 2016

Yes, I thought this myself, but then I discovered that this is rarely the case (I worked with images and difference in performance between polymorphic/concrete function was about 5 times). I also read a blog post by author of HLearn where he notes that idealy there should be no difference in performance between polymorphic and concrete functions, but it's not the case with current GHC.

@cocreature
Collaborator

Interesting, thanks for all the help, I’ll definitely try it out and see how it improves the benchmarks.

@simonmichael
Owner

Great! Thank you both for looking at this.

I'm all for megaparsec 5, even if it has to be an extra-dep in the stack file. The downside of leaving stackage LTS is it makes hledger harder to package, and hurts our availability on Debian and other unix distros, which is not good. On the other hand it looks like no version of megaparsec is in Debian yet..

I agree that supporting both 4 and 5 is too much work, unless the essential performance improvements in 5 were small and could be shipped in hledger as a patch to megaparsec 4 (extra work).

Another option is stay with 4 a little longer but do some optimisation of hledger's parser. I'd love to know what causes so much allocation - too much backtracking ? This must be done sooner or later, but it's significant work.

Let's assume we can require megaparsec 5.

@cocreature
Collaborator

Alright, I just pushed the megaparsec 5 changes and specialization.

It seems to be about on par with the parsec version using lts-6.0 (might be very slightly faster) and a bit faster using a nightly snapshot. I suspect the latter is because of the transformer changes but it might very well also be something totally unrelated. Overall I think this is good enough to merge it.

We should definitely try to investigate how we can make the parser faster since looking at profiling outputs it takes up most of the runtime, but that’s a separate issue.

@simonmichael
Owner

Much better. Here I got
Time:

+--------------------------------------++----------------+---------------------+---------------------+
|                                      || hledger.parsec | hledger.megaparsec4 | hledger.megaparsec5 |
+======================================++================+=====================+=====================+
| stats -f data/100x100x10.journal     ||           0.05 |                0.06 |                0.06 |
| stats -f data/1000x1000x10.journal   ||           0.23 |                0.28 |                0.21 |
| stats -f data/10000x1000x10.journal  ||           1.90 |                2.68 |                1.92 |
| stats -f data/100000x1000x10.journal ||          20.32 |               27.73 |               20.41 |
+--------------------------------------++----------------+---------------------+---------------------+

Memory (hledger.megaparsec5):

   2,619,886,528 bytes allocated in the heap
     209,411,616 bytes copied during GC
      34,485,360 bytes maximum residency (10 sample(s))
       2,489,336 bytes maximum slop
              81 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0      5061 colls,     0 par    0.296s   0.313s     0.0001s    0.0021s
  Gen  1        10 colls,     0 par    0.141s   0.177s     0.0177s    0.0781s

  TASKS: 4 (1 bound, 3 peak workers (3 total), using -N1)

  SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.000s  (  0.001s elapsed)
  MUT     time    1.409s  (  1.454s elapsed)
  GC      time    0.437s  (  0.490s elapsed)
  EXIT    time    0.001s  (  0.006s elapsed)
  Total   time    1.849s  (  1.951s elapsed)

  Alloc rate    1,859,634,479 bytes per MUT second

  Productivity  76.3% of total user, 72.3% of total elapsed
@simonmichael simonmichael commented on the diff Jul 11, 2016
hledger-lib/hledger-lib.cabal
, regex-tdfa
, safe >= 0.2
+ , semigroups
@simonmichael
simonmichael Jul 11, 2016 Owner

Is semigroups required ? I don't see where it's used.

@cocreature
cocreature Jul 11, 2016 Collaborator

It is used in Hledger.Read.Common.hs in Data.List.NonEmpty. It moved to base in ghc 8, but you probably want to support older GHCs as well.

@simonmichael
simonmichael Jul 11, 2016 Owner

Aha, thanks. Yes I do, which GHC(s) are you testing with, by the way ?

@cocreature
cocreature Jul 11, 2016 Collaborator

I tested 7.10 and 8.0.

@simonmichael
Owner

Lots of work! Looking good, there's just some functional tests failing (make [func]test).

@cocreature
Collaborator

Looking good, there's just some functional tests failing (make [func]test).

Ah sorry I missed these. Now there are only two failures left, one is also happening on master but the other one (include.test:2) seems to be an actual bug, but sadly I have no idea what’s causing it.

@cocreature
Collaborator

Alright, I figured out the problem. Compare the following to statements

> runParser (((modifyState (+1) >> fail "") <|> modifyState (+1)) >> getState) 0 "" ""
Right 1

and

> flip evalState 0 $ runParserT (((modify (+1) >> fail "") <|> modify (+1)) >> get) "" "" :: Either (ParseError Char Dec) Int
Right 2

We need to reset the state when backtracking like parsec does. This can be accomplished by simply swapping the order of monad transformers. I’ll try to commit a fix during the next few days.

@cocreature
Collaborator

Done, all functional tests are now passing except for one that also fails on master so it is unrelated.

@simonmichael
Owner

Nice.

Parsec's support user state seems to have been good for code simplicity and perhaps performance. With megaparsec, we have a lot more lifting, and evaluating the parser monad stacks is a bit more daunting and error prone.

I'm ready to merge this, however since it doesn't yet change anything for users, I think I should first ship the imminent 0.28 release, still based on parsec, keeping life simple for packagers.

@cocreature
Collaborator

Fine with me, but please try to avoid large changes for the 0.28 release until you merge this since keeping large PRs up2date is quite painful.

@simonmichael
Owner

Yes entirely agreed. I'll freeze all this stuff until this PR is merged.

@simonmichael simonmichael modified the milestone: 1.0 Jul 17, 2016
cocreature added some commits Jul 10, 2016
@cocreature cocreature Replace Parsec with Megaparsec (see #289)
This builds upon PR #289 by @rasendubi
bfa40da
@cocreature cocreature Revert renaming of parseWithState to parseWithCtx e39a7ec
@cocreature cocreature Fix doctests 0721577
@cocreature cocreature Update for Megaparsec 5 c4d4d2a
@cocreature cocreature Specialize parser to improve performance bd2053b
@cocreature cocreature Pretty print errors ed15d72
@cocreature cocreature Swap StateT and ParsecT
This is necessary to get the correct backtracking behavior, i.e. discard
state changes if the parsing fails.
55137e2
@cocreature
Collaborator

Rebased on master since it was no longer mergeable.

@simonmichael simonmichael merged commit 4141067 into simonmichael:master Jul 29, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@simonmichael
Owner
simonmichael commented Jul 29, 2016 edited

Ack.. I don't know how that happened, but since the 0.28 tasks keep coming, it's going to keep happening. So I have just gone ahead and merged this. Thank you!

@simonmichael simonmichael modified the milestone: 1.0, post 1.0 Oct 31, 2016
@mstksg mstksg added a commit to mstksg/hledger that referenced this pull request Feb 3, 2017
@cocreature @mstksg cocreature + mstksg Replace Parsec with Megaparsec (see #289) (#366)
* Replace Parsec with Megaparsec (see #289)

This builds upon PR #289 by @rasendubi

* Revert renaming of parseWithState to parseWithCtx

* Fix doctests

* Update for Megaparsec 5

* Specialize parser to improve performance

* Pretty print errors

* Swap StateT and ParsecT

This is necessary to get the correct backtracking behavior, i.e. discard
state changes if the parsing fails.
291b5a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment