This repository has been archived by the owner. It is now read-only.

Fix performance regression introduced in 6.8.2 #63

Merged
merged 1 commit into from Jul 3, 2016

Conversation

Projects
None yet
3 participants
@danez
Copy link
Member

danez commented Jul 2, 2016

This commit e6c11a0 (#19) made a big performance regression.
The reason was that parseConditional was always cloning the current state even if no questionmark (potential conditional or flow-optional
token) was at the current position.
Simply checking if questionmark matches the current token solves the problem.

Fixes #62

Testcase:

Node: v7.0.0-nightly20160621ecc48a154d
Testfile:
https://gist.githubusercontent.com/milafrerichs/69035da4707ea51886eb/raw/4cb1783c2904f52cbb8a258ee96031f9054d155b/eu.topojson

Before:
bildschirmfoto 2016-07-02 um 17 09 47

After:
bildschirmfoto 2016-07-02 um 17 08 18

@danez danez added the Tag: Bug Fix label Jul 2, 2016

@danez danez force-pushed the danez:performance-regression branch from 27aec6f to ed63df9 Jul 2, 2016

@sophiebits

This comment has been minimized.

Copy link

sophiebits commented Jul 2, 2016

Cool, glad there's a simple fix. I suppose this still could be a problem on files with thousands of question marks, but maybe that's not worth worrying about. Should be fine on JSON-like things which are the most likely contents of a large JS file.

@danez

This comment has been minimized.

Copy link
Member Author

danez commented Jul 2, 2016

Yes, I want to run more profiling on babylon, I saw some possible candidates, but I need to check them. Gladly it is really nice, easy and fun to do profiling with chrome dev tools and node 7.

@sophiebits

This comment has been minimized.

Copy link

sophiebits commented Jul 2, 2016

Another thought I just had: if I try to parse a ? b : c ? d : e ? f : g ? h : i ? j : k ? l : m ? n : o ? p : syntax error, will it try exponentially many paths when backtracking?

@danez

This comment has been minimized.

Copy link
Member Author

danez commented Jul 2, 2016

That might be related to #59. I can check tomorrow.

@danez danez force-pushed the danez:performance-regression branch from ed63df9 to c562e22 Jul 3, 2016

Daniel Tschinder
Fix performance regression introduced in 6.8.2
This commit e6c11a0 (#19) made a big performance regression.
The reason was that parseConditional was always cloning the current state
even if no question mark (potential conditional or flow-optional
token) was at the current position.
Simply checking if questionmark matches the current token solves the problem.

Fixes #62

@danez danez force-pushed the danez:performance-regression branch from c562e22 to 4e2072d Jul 3, 2016

@danez

This comment has been minimized.

Copy link
Member Author

danez commented Jul 3, 2016

Oh sorry I was reading it on the phone yesterday and thought you said that you get an syntax error when parsing nested ternaries. :)

In the given example there would be no backtracking involved now with this PR (i added one more check), as the flow optional operator can only be inside parentheses (a?).
If your example would have parens around every ternary then it would do n backtracks where n = number of nested ternaries if I'm not mistaken, which should be linear.
Basically the flow plugin is only wrapping the parsing of conditionals into a try-catch-clause and doing a backtrack when a SyntaxError happens, so that other parts of the flow plugin can validate it as valid optional param.

@kittens kittens merged commit 22cf1f8 into babel:master Jul 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kittens

This comment has been minimized.

Copy link
Contributor

kittens commented Jul 3, 2016

Thanks! Published as 6.8.3

@sophiebits

This comment has been minimized.

Copy link

sophiebits commented Jul 3, 2016

Thank you!

@danez danez deleted the danez:performance-regression branch Jul 5, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.