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

avoid exceptions for control flow #9974

Merged
merged 3 commits into from May 21, 2019

Conversation

matthewrobertson
Copy link
Contributor

Q                       A
Fixed Issues? Fixes #9972
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link N/A
Any Dependency Changes? No
License MIT

This commit optimizes the performance of the typescript parser plugin by avoiding exceptions for control flow. Currently, the typescript parser is filled with code branches that look something like this:

const state = this.state.clone();
try {
  this.parseSomething();
} catch (e) {
  if (!(e instanceof SyntaxError)) {
    // something is actually wrong
    throw e;
  }
  // we are just bailing out of a lookahead branch
  this.state = state;
}

Throwing exceptions in node is expensive so this commit adds a few conditionals to avoid throwing where possible. The most common issue was related to type argument parsing, which had many false positives due to less-than binary expressions (eg const y = x < could be const y = x <z>(); but it much more likely to be const y = x < 10;), so I added a new tsTryParseTypeArguments that resets state and returns undefined instead of throwing.

@babel-bot
Copy link
Collaborator

babel-bot commented May 14, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10826/

this.state = state;
return undefined;
}

Copy link
Member

@danez danez May 16, 2019

Choose a reason for hiding this comment

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

This is not nice, as we have this code now twice. But I see why you have done this.

What you should consider here is that cloning the state is also something that is more on the slow side, but not sure how it compares to throwing exceptions.

Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

Happy to merge as is.

if (this.eatRelational(">")) {
return this.finishNode(node, "TSTypeParameterInstantiation");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can lines 1478-1496 be shared with the tsParseTypeArguments method?

@matthewrobertson
Copy link
Contributor Author

Now that #9989 has landed the optimizations here are not as urgent, so I have updated it to remove the tsTryParseTypeArguments and left only the simpler optimizations where we could avoid exceptions in cases where the current state would always throw.

If there is interest, I can change tsParseTypeArguments to an implementation that never throws, but I think we should do that in a separate PR and completely replace the existing method instead of adding a duplicate implementation (as was the case originally in the first version of this PR).

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Let's merge this for now, its better than before anyway.

PS. If you like perf optimizations, they are always welcome 😛

@nicolo-ribaudo nicolo-ribaudo merged commit 9c06e4e into babel:master May 21, 2019
@matthewrobertson matthewrobertson deleted the parser-perf branch May 21, 2019 19:19
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: perf area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parser performance is very bad when typescript is enabled
4 participants