Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

☄ Make tokens optional to improve performance #563

Merged
merged 1 commit into from
Jun 28, 2017
Merged

☄ Make tokens optional to improve performance #563

merged 1 commit into from
Jun 28, 2017

Conversation

danez
Copy link
Member

@danez danez commented Jun 4, 2017

Q A
Bug fix? no
Breaking change? yes
New feature? yes
Deprecations? no
Spec compliancy? no
Tests added/pass? yes
Fixed tickets Fixes #458
License MIT

Adding tokens to the ast is significant slower and most tools
don't ever use them anyway. Babel uses it so we need to enable it there.

With tokens:

name                            run  
./fixtures/angular.js           341ms
./fixtures/backbone.js          33ms 
./fixtures/ember.debug.js       884ms
./fixtures/jquery.js            171ms
./fixtures/react-with-addons.js 361ms

Without:

name                            run  
./fixtures/angular.js           262ms
./fixtures/backbone.js          29ms 
./fixtures/ember.debug.js       686ms
./fixtures/jquery.js            129ms
./fixtures/react-with-addons.js 281ms

@hzoo
Copy link
Member

hzoo commented Jun 5, 2017

What does Babel use it for anyway?

@danez
Copy link
Member Author

danez commented Jun 5, 2017

The generator seems to use it for whitespace stuff, https://github.com/babel/babel/blob/02473a72c160545ed225482515b569d26fc41ea5/packages/babel-generator/src/index.js#L14

I just had an idea, what if we instead of babel-generator use prettier. Have you tried that? I wonder if that works. :)

@xtuc
Copy link
Member

xtuc commented Jun 5, 2017

What if we expose the option as a Babylon plugin? This would allow to push this plugin from a Babel plugin if it needs it.

@hzoo
Copy link
Member

hzoo commented Jun 5, 2017

@danez It a lot slower though (forgot how much but at least >2x). I definetely wouldn't be opposed to it (I believe someone brought it up earlier so I tested a while ago). prettier = specific printer just like babel-generator. Would need to figure out how to ignore some of the logic that may make it slower than just simply printing it. Would be a nice way for all of us to use one codebase. It's as simple as generatorOps.generator = prettier in the babel options (assuming we don't remove the babel nodes as you mention in prettier/prettier#1940 (unless someone will change all the nodes in our codebase as well as all the 3rd party plugins with a codemod)

https://github.com/fkling/astexplorer/blob/2dca059af77c979b69b84d16454409e5f6465b02/website/src/parsers/js/transformers/babel7/index.js#L26-L46

cc @vjeux @existentialism

@vjeux
Copy link

vjeux commented Jun 5, 2017

There's probably a lot of low hanging fruit for making prettier fast by the way. Performance has been okay for our use case so far so we didn't look into it at all.

But, it's likely never going to be as fast as the current printer since it has to do more work.

@hzoo
Copy link
Member

hzoo commented Jun 5, 2017

I was thinking we could make some of it's operations no-op (do the default print) if we don't care about specific formatting that can be slow but not sure if that is viable

@vjeux
Copy link

vjeux commented Jun 5, 2017

Why would you use prettier if you're not going to use its 80-columns formatting abilities? It seems like it defeats the purpose of the tool.

Also, I'm not really sure that the overhead of prettier is really a good idea. I don't think that printing code in a human-looking way is worth spending more time. You pay the cost of printing every single time, you only look at the output once in a while.

@hzoo
Copy link
Member

hzoo commented Jun 5, 2017

Was just whether it would be worth looking into perf issues so we don't have maintain 2 printers

@vjeux
Copy link

vjeux commented Jun 5, 2017

Oh I see. I didn't consider the overhead of maintenance.

@hzoo
Copy link
Member

hzoo commented Jun 6, 2017

Ok I totally ignored your "The generator seems to use it for whitespace stuff, " @danez

Let's just remove the usage in Babel then. We don't care about the output format and we shouldn't encourage anyone else to care about it either (will be minified, etc). We got the quotes option to be removed earlier (babel/babel#5154). So same idea. Just hardcode the whitespace/quote stuff.

Ok nvm the printer is using tokens so need to figure out how do that for newlines without it?

Adding tokens to the ast is significant slower and most tools
don't ever use them anyway
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faster without tokens array
4 participants