Skip to content

Remove all *Rec combinators #173

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

Closed
wants to merge 2 commits into from
Closed

Remove all *Rec combinators #173

wants to merge 2 commits into from

Conversation

jamesdbrock
Copy link
Member

@jamesdbrock jamesdbrock commented Apr 2, 2022

We don't need *Rec combinators anymore because the CPS ParserT is always stack-safe.

Also add a bench which would blow a stack-unsafe parser.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@jamesdbrock jamesdbrock marked this pull request as draft April 2, 2022 10:45
@jamesdbrock jamesdbrock changed the title bench which would blow a stack-unsafe parser Remove all *Rec combinators Apr 2, 2022
We don't need these anymore because the CPS ParserT is always
stack-safe.
@jamesdbrock jamesdbrock marked this pull request as ready for review April 2, 2022 12:27
@natefaubion
Copy link
Contributor

As I stated elsewhere, I think we need to be careful about just removing these. In my testing manyRec is several times faster than many as it’s iterative. That would suggest to me that it would extend to anything implemented in terms of many/manyRec. It would be useful to have that benchmark.

@jamesdbrock
Copy link
Member Author

Oh yeah you said that here.

#154 (comment)

Not necessarily, or at least I don't think you should just remove the Rec implementations without testing. The parser is always stack-safe now, yes, but the iterative implementations may be more efficient at runtime. For example, the upstream many combinator is significantly slower than manyRec. It may be the case that we should copy over the Rec implementations and remove the calls to tailRecM, just using standard Monadic recursion. It could also be the case that the "naive" implementations perform fine in comparison and we should drop the Rec implementaiton.

@jamesdbrock
Copy link
Member Author

Yeah I see that too.

runParser many digit 5000
mean   = 37.42 ms
stddev = 13.66 ms
min    = 29.48 ms
max    = 106.74 ms
runParser manyRec digit 5000
mean   = 12.13 ms
stddev = 4.56 ms
min    = 10.25 ms
max    = 39.80 ms

@jamesdbrock
Copy link
Member Author

Okay... I'm going to add some benchmarks to get some better visibility into this.

@jamesdbrock
Copy link
Member Author

Discussion continues in #177

@jamesdbrock jamesdbrock closed this Apr 3, 2022
@jamesdbrock jamesdbrock deleted the benchstack branch April 5, 2022 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants