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

Only compute Position if not already in state #9989

Merged
merged 2 commits into from May 17, 2019
Merged

Only compute Position if not already in state #9989

merged 2 commits into from May 17, 2019

Conversation

danez
Copy link
Member

@danez danez commented May 16, 2019

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

Because of #9974 I was curious why throwing an exception is slow. So I did some investigation and my first guess was maybe we do something really slow when throwing. Turns out we do. 😫Every time we throw an exception we do calculate the line and column based on the current position (index) in the input string.

So I was curious if that can be improved and after looking through StackOverflow for a long time :P I couldn't get this algorithm (https://github.com/babel/babel/blob/master/packages/babel-parser/src/util/location.js#L41) any significant faster.

For a short time I thought I got it faster by using substring() + split('\n') instead of the for + regex, but the speed improvement was only because I basically removed support for line endings besides \n. After changing to split(regex) it was slow again.

I was thinking more about it and I realized that actually most errors are thrown right when encountered, means the state is currently pointing to the error position. In this case and even in the case when the error is reported for the last token, we can easily reuse the location that is already in the state. No need for calculation at all.

All benchmarks are node 12.2.0

Before
┌──────────────────────────┬───────────────────────────────┬─────────────────────────────┬─────────────────────────────┐
│ fixture                  │ babel                         │ babelWithFlow               │ babelWithTS                 │
├──────────────────────────┼───────────────────────────────┼─────────────────────────────┼─────────────────────────────┤
│ es5/angular.js           │ 26.54 ops/sec ±4.28% (38ms)   │ 11.1 ops/sec ±3.25% (90ms)  │ 4.76 ops/sec ±2.89% (210ms) │
├──────────────────────────┼───────────────────────────────┼─────────────────────────────┼─────────────────────────────┤
│ es5/ember.debug.js       │ 10.56 ops/sec ±2.77% (95ms)   │ 4.4 ops/sec ±2.91% (227ms)  │ 1.04 ops/sec ±9.82% (964ms) │
├──────────────────────────┼───────────────────────────────┼─────────────────────────────┼─────────────────────────────┤
│ es5/babylon-dist.js      │ 44.52 ops/sec ±5.35% (22ms)   │ 18.07 ops/sec ±2.87% (55ms) │ 16.17 ops/sec ±2.41% (62ms) │
├──────────────────────────┼───────────────────────────────┼─────────────────────────────┼─────────────────────────────┤
│ es5/jquery.js            │ 52.38 ops/sec ±5.04% (19ms)   │ 20.95 ops/sec ±4.67% (48ms) │ 16.15 ops/sec ±3.33% (62ms) │
├──────────────────────────┼───────────────────────────────┼─────────────────────────────┼─────────────────────────────┤
│ es5/backbone.js          │ 229 ops/sec ±24.19% (4.359ms) │ 90.2 ops/sec ±13.65% (11ms) │ 90.85 ops/sec ±2.16% (11ms) │
├──────────────────────────┼───────────────────────────────┼─────────────────────────────┼─────────────────────────────┤
│ es5/react-with-addons.js │ 24.81 ops/sec ±2.63% (40ms)   │ 10.44 ops/sec ±3.12% (96ms) │ 7.17 ops/sec ±2.35% (140ms) │
└──────────────────────────┴───────────────────────────────┴─────────────────────────────┴─────────────────────────────┘
After
┌──────────────────────────┬───────────────────────────────┬──────────────────────────────┬──────────────────────────────┐
│ fixture                  │ babel                         │ babelWithFlow                │ babelWithTS                  │
├──────────────────────────┼───────────────────────────────┼──────────────────────────────┼──────────────────────────────┤
│ es5/angular.js           │ 27.69 ops/sec ±4.56% (36ms)   │ 11.52 ops/sec ±3.56% (87ms)  │ 12.58 ops/sec ±2.87% (79ms)  │
├──────────────────────────┼───────────────────────────────┼──────────────────────────────┼──────────────────────────────┤
│ es5/ember.debug.js       │ 11.07 ops/sec ±3.31% (90ms)   │ 4.48 ops/sec ±3.77% (223ms)  │ 4.95 ops/sec ±2.51% (202ms)  │
├──────────────────────────┼───────────────────────────────┼──────────────────────────────┼──────────────────────────────┤
│ es5/babylon-dist.js      │ 46.94 ops/sec ±1.58% (21ms)   │ 18.49 ops/sec ±2.76% (54ms)  │ 20 ops/sec ±4.82% (50ms)     │
├──────────────────────────┼───────────────────────────────┼──────────────────────────────┼──────────────────────────────┤
│ es5/jquery.js            │ 56.61 ops/sec ±1.66% (18ms)   │ 21.94 ops/sec ±4.18% (46ms)  │ 23.57 ops/sec ±4.7% (42ms)   │
├──────────────────────────┼───────────────────────────────┼──────────────────────────────┼──────────────────────────────┤
│ es5/backbone.js          │ 230 ops/sec ±28.92% (4.351ms) │ 95.37 ops/sec ±13.28% (10ms) │ 108 ops/sec ±1.79% (9.263ms) │
├──────────────────────────┼───────────────────────────────┼──────────────────────────────┼──────────────────────────────┤
│ es5/react-with-addons.js │ 26.29 ops/sec ±3.04% (38ms)   │ 10.98 ops/sec ±3.16% (91ms)  │ 12.07 ops/sec ±2.86% (83ms)  │
└──────────────────────────┴───────────────────────────────┴──────────────────────────────┴──────────────────────────────┘

So together with #9974 this should give a good boost for ts in certain situations

@matthewrobertson Can you check if that improves your reported case even further?

@danez danez added area: perf PR: Polish 💅 A type of pull request used for our changelog categories pkg: parser area: typescript labels May 16, 2019
@danez danez changed the title Only compute Position if not already in state and optimize line calculation Only compute Position if not already in state May 16, 2019
@babel-bot
Copy link
Collaborator

babel-bot commented May 16, 2019

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

@babel-bot
Copy link
Collaborator

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

@matthewrobertson
Copy link
Contributor

Nice find @danez! I think with this change we might not need #9974. I did some simple profiling and it looks like with this change alone we get most of the perf benefits that would be gained from that PR:

parser config parse time
no typescript 2.25s
with typescript 8.05s
with typescript AND #9974: 3.33s
with typescript AND #9989: 3.18s
with typescript AND #9989 AND #9974 2.59s

So there is some additional benefit from adding #9974 to this, but this change alone is better than #9974 alone. Maybe I can trim that PR down a bit so we just take some of the simpler optimizations that don't add too much complexity.

@danez
Copy link
Member Author

danez commented May 16, 2019

Maybe I can trim that PR down a bit so we just take some of the simpler optimizations that don't add too much complexity.

I think your PR is still very valuable and I don't see why we shouldn't bail out as soon as possible rather than try parsing and reverting state etc.

@danez danez merged commit b1826bf into babel:master May 17, 2019
@danez danez deleted the faster-errors branch May 17, 2019 22:32
@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 PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants