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

Amp-bind: Rearrange grammar rules and regenerate parser #8046

Merged
merged 9 commits into from Mar 10, 2017

Conversation

kmh287
Copy link
Contributor

@kmh287 kmh287 commented Mar 9, 2017

  • Modified the performance example to use different expressions per binding to circumvent the new caching strategy
  • Updated the grammar in bind-expr-impl.jison to remove FALSE, TRUE, and NULL. Is there any reason to support these? They're not supported in js.
  • Updated the grammar in bind-expr-impl.jison by rearranging the tokens. The parser checks every potential token against this list in order, and so I've arranged them to try to put the more likely ones first. e.g. a NAME is probably more likely to come up than the mod operator. That being said, this is just my guess and I'm open to suggestions.
  • Regenerated bind-expr-impl.js based on the above changes. All changes in that file were made by jison, except changing the copyright date at the top.

With the changes to the performance HTML file and the parser, the time it takes Bind to parse bindings on performance.html decreased by 10% on my machine. (Chrome 56, CPU throttling at 10x, two different experiments with 10 tries each comparing master's bind-expr-impl.js to the one on this branch, web-worker experiment disabled)

/to @choumx, @jridgewell

@kmh287 kmh287 changed the title Bind: Rearrange grammar rules and regenerate parser Amp-bind: Rearrange grammar rules and regenerate parser Mar 9, 2017
@@ -884,4 +884,20 @@ Parser.prototype = parser;parser.Parser = Parser;
return new Parser;
})();

exports.parser = parser;

if (typeof require !== 'undefined' && typeof exports !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Contributor Author

@kmh287 kmh287 Mar 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's part of the autogenerated file. I'm not sure if they added this in the newest version of jison or if @choumx just removed it from previous versions.

EDIT: Judging from the CI error it's the latter. I'll remove it now.

@kmh287 kmh287 merged commit d09c3e3 into ampproject:master Mar 10, 2017
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants