-
Notifications
You must be signed in to change notification settings - Fork 211
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
Left factor parser for function types #606
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #108 This gives a massive (~30x) parsing performance improvement for the benchmark code from the above issue: ``` benchmarking Issue #108/Text time 169.9 ms (167.6 ms .. 172.8 ms) 1.000 R² (0.999 R² .. 1.000 R²) mean 174.8 ms (172.6 ms .. 177.3 ms) std dev 3.525 ms (2.008 ms .. 5.131 ms) variance introduced by outliers: 12% (moderately inflated) time 5.860 ms (5.826 ms .. 5.904 ms) 1.000 R² (1.000 R² .. 1.000 R²) mean 5.921 ms (5.902 ms .. 5.939 ms) std dev 56.42 μs (46.69 μs .. 67.52 μs) ``` The root cause was that the parser for function types was introducing excessive backtracking. This led to parsing performance being exponential in the number of atomic `operatorExpression`s. This change left-factors the `operatorExpression` parser by gutting `annotatedExpression`. Specifically, this moves the logic for parsing annotated `List`/`Optional`/`merge` into `primitiveExpression` and then consolidating the remaining logic for parsing an ordinary `Annot` into `operatorExpression` so that it no longer has to backtrack.
This was referenced Sep 27, 2018
Closed
Note that still needs a little cleanup before merging because it discards the |
I can confirm that this also fixes #580 🎉 Running time with this branch is ~1s, which I'd consider good enough. Thanks for the good work @Gabriel439 and @phadej 👏 |
This was the pathological example that helped quickly narrow down the problem
`alternative4` now subsumes `alternative5`
…hall-Library into gabriel/left_factor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #108
This gives a massive (~30x) parsing performance improvement for the benchmark
code from the above issue:
After:
The root cause was that the parser for function types was introducing
excessive backtracking. This led to parsing performance being exponential
in the number of atomic
operatorExpression
s.This change left-factors the
expression
parser by guttingannotatedExpression
. Specifically, this moves the logic for parsing unannotatedList
/Optional
/merge
intoexpression
and then consolidates theremaining logic for parsing an ordinary
Annot
intoexpression
, applyinga fixup for unannotated
List
/Optional
/merge
expressions to tag them withtheir annotation.