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

Fix performance bug: Bad value context for argument value #332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NightRa
Copy link

@NightRa NightRa commented Sep 13, 2016

Using arguments in a dynamic way in a function leads to the JIT (v8) not optimizing the whole function, with the error: "Bad value context for argument value". See GoogleChrome/devtools-docs#53 (comment) and Note gist.

This commit wraps the parse function with handling of arguments, and calls into the regular function with the args array, and now it can be optimized.

This fix improved performance almost 2x for me, from 17s to 9s. (for 500Kb of text so there's still room for improvement)

@GerHobbelt
Copy link
Contributor

@NightRa : nice catch, this one! Since you seem interested in maximum performance jison parsers, you may be interested in GerHobbelt@d2f8af7
(with some more performance-related work done in
GerHobbelt@e44add3 ).

The thought there is that

  • creating a function wrapped in another plus bind() is also rather costly (jsperf is down unfortunately, but old tests there indicated that .bind() is not exactly the fastest kid on the block)
  • we already know exactly which extra arguments will be passed to the parse() API as these arguments are listed in the grammar file in the %parse-params option line

==> that made me wonder: "Why don't we take the %parse-params argument list and inject straight away into the code instead of messing with this arguments semi-array at all?" and those two referenced commits above are the result.


Thanks you for your work; I had not realized the high cost of arguments+slice in the parser core yet. 👍

GerHobbelt added a commit to GerHobbelt/jison that referenced this pull request Nov 11, 2016
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.

2 participants