Skip to content
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

Remove usage of Function.prototype.toString() #998

Closed
27 of 28 tasks
bd82 opened this issue Aug 12, 2019 · 2 comments
Closed
27 of 28 tasks

Remove usage of Function.prototype.toString() #998

bd82 opened this issue Aug 12, 2019 · 2 comments

Comments

@bd82
Copy link
Member

bd82 commented Aug 12, 2019

Tasks

Initial Feature.

  • Poc Concept (Remove Usage of Function.prototype.toString #995)
  • Resolve runtime performance Regressions (Remove Usage of Function.prototype.toString #995).
    • Move "ToFastProps" to self Analysis phase.
    • Investigate performance regression for complex grammars
  • Implement Semantic Actions Wrapper.
  • JSDocs for Semantic Actions Wrapper.
  • Implement NOOP replacements for additional Parser methods ( LA / BACKTRACK / ...?)
  • Ensure returned value of the "Recorder methods" is as close as possible to the regular methods.
    - Will reduce "recording failures"
  • Implement Flag for "Recording Phase"
  • Provide a useful error message if a recording failure occurred.
    • Invalid SUBRULE Arg.
    • Invalid CONSUME Arg.
    • Some other exception.
  • All Unit tests passing.
  • 100% Coverage
  • All Integration tests passing.
  • Inspect cold start performance changes.

Features to Remove

  • Grammar de-Serialization
    - The serialization itself is useful for diagrams generation.

Docs

  • Update Tutorial.
  • Separate In-Depth for embedded actions and possible problems:
    - Code that would throw exceptions during parsing.
    - Code that modifies global/instance state.
  • Update FAQ parts for minification/bundling.
  • Deprecate Webpack / Minification examples.
  • Update Breaking Changes.
  • Update ChangeLog
  • Remove limitations related to order of properties (SEP / NAME?)

Post Release

  • Would the playground need $.ACTION wrapper anywhere?

New Examples:

  • Grammar Composition / Macros - defining new parsing methods by end users.
  • Evaluate a more "dynamic Grammar" example.

Out of scope

Potential Upgrades ( For the future in new issues)

  • Automatic ignoring of ambiguities that have a user defined GATE.
  • Rule/Token names should no longer be limited to ASCI. (Unicode Rule names).
  • d.ts generation for the CST.
  • Macros in general.
  • Evaluate replacing AT_LEAST_ONE/MANY_SEP with a Macro
  • Leverage the Record Phase to implement runtime checks.

The Problem

The use of Function.prototype.toString() to analyze the grammar causes many issues.

These issues are primarily with bundling which may modify the source code of the grammar (webpack/babel/...). but also by adding constraints and limitations to Chevrotain capabilities/features because the current analysis being done is very limited, e.g:

  • In MANY_SEP the SEP must appear first otherwise we cannot read it.
  • User defined Grammar utilities (Grammar Composition) are not possible because the self analysis logic is limited to known DSL method names, e.g:
    •      class MyParser extends CstParser {
                  TWICE(tok) {
                       this.CONSUME(tok);
                       this.CONSUME(tok);
                  }
           }
  • Generation of d.ts signatures for the CST was blocked due to limited analysis information (See Generation of d.ts signatures for CST Parsers #851)

The Solution

@blainehansen (#683) and @EqualMa (#992) Suggested that the Grammar Structure may be "recorded" instead of being parsed from the source text (Function.toString).

This means the grammar rules would be executed with a different implementation of the Chevrotain engine, during this alternative execution the relevant information would be collected and the GAST nodes would be created.

This is a great idea 👍 which may also provide cold start performance benefits.

There is one major issue with this concept which is that during the "recording" phase, end user's code may throw a runtime exception thus failing the recording. The possible workaround for this is to provide a wrapper for embedded actions which would not be executed during recording phase.

This would be a breaking change, however CstParsers should not have many issues because
by definition they have very few (if any) semantic actions.
And even in the case of EmbeddedActionsParsers only some semantic actions could cause issues during recording...

@bd82
Copy link
Member Author

bd82 commented Aug 17, 2019

Cold Start performance is now ~20% faster
The GAST Building seems to take take at most 15% of the cold start time so GAST serialization seems to have no purpose as it would provide negligible gains for cold start performance.

@bd82
Copy link
Member Author

bd82 commented Aug 18, 2019

Now at the hardest part, the documentation phase... :(

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

No branches or pull requests

1 participant