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

Discussion: Babel plugin to optimize javascript engine code (like v8) #9

Open
hzoo opened this issue Jan 3, 2017 · 13 comments
Open

Comments

@hzoo
Copy link
Member

hzoo commented Jan 3, 2017

EDIT: can also be an eslint rule/plugin or a combination of both

Pardon my inexperience/lack of knowledge in all of this but wanted to get some ideas down so we can all learn!

Also ref the thread: https://twitter.com/left_pad/status/814967401276182529
Like in: http://richardartoul.github.io/jekyll/update/2015/04/26/hidden-classes.html

Always instantiate your object properties in the same order so that hidden classes, and subsequently optimized code, can be shared.
Adding properties to an object after instantiation will force a hidden class change and slow down any methods that were optimized for the previous hidden class. Instead, assign all of an object’s properties in its constructor.
Code that executes the same method repeatedly will run faster than code that executes many different methods only once (due to inline caching).

We should be able to lint/add a runtime check for these?

1  function Point(x,y) {
2    this.x = x;
3    this.y = y;
4  }
5 
7  var obj1 = new Point(1,2);
8
9 obj1.a = 5;

So for the above example we could make a few warnings/errors.

We could warn that a property a is being created on line 9 but the constructor starting on line 1 doesn't contain this.a? So it could be added there, or ask that it be not used if possible?

because

10 obj1.a = 5;
11 obj1.b = 10;
12
13 obj2.b = 10;
14 obj2.a = 5;

have different transition paths we can warn on this kind of thing (moving it to the constructor should fix it as well?)

In some cases we can do a static check for these kinds of things but otherwise we will probably want to just do some code instrumentation around objects/etc to do runtime check/analysis instead?

Maybe we can use something like ES2015 Proxy (or just wrap certain variables in functions), or is it necessary to instrument v8 as well or use something like IRHydra?

Might be cool to see some data around the kinds of objects created (how many times) and what properties are available and added/removed at what lines in the codebase.

Other deopts; https://github.com/petkaantonov/bluebird/wiki/Optimization-killers

cc @addyosmani, @lelandrichardson, @jordwalke

@Jessidhia
Copy link
Member

Jessidhia commented Jan 3, 2017

The easiest one to add, probably in babel-eslint, would be warning on delete with a MemberExpression argument ("delete properties"), as it's always a deoptimization.

Objects that are being used like that should be replaced by a Map or Set. Sometimes you do have to do it when interacting with third party code that does use a "hash object", though.

Warning for property add will either require wrapping everything in Proxy, or wrapping every [[Set]] with a function call...

@lelandrichardson
Copy link

I'm not sure whether this makes more sense as a babel plugin or an eslint plugin. I feel like eslint might make more sense, since the best thing is probably to let the user know something that they might not know but impacts performance, and let them decide the best way to fix...

Some thoughts on things that might maybe seem catchable by a linter:

  1. using __proto__ object literal
  2. deep logic exit conditions as described here
  3. leaking arguments
  4. any unsafe arguments usage
  5. only optimized for ... in usage
  6. assigning to class properties not assigned to in the constructor
  7. having variables change their backing type (ie, going from int32 to double by having var a = 1; and then later a = 0.5;)
  8. object properties that automatically deopt to a hashtable (ie, var foo = { 'a.b': c };)
  9. using anything unsupported such as debugger, eval, with, etc.
  10. suggest moving larger functions with try { ... } catch () { ... } into a non-try-catched function with a wrapper function calling it from within a try-catch.

@Jessidhia
Copy link
Member

Jessidhia commented Jan 5, 2017

@lelandrichardson no. 10 in there could maybe be ignored in async contexts, at least when optimizing for v8. It'll be either in code transpiled into a Promise-based driver (no try/catch), or be executed by Ignition/TurboFan. Or a syntax error 😄

@hzoo
Copy link
Member Author

hzoo commented Jan 5, 2017

Either ESLint/Babel works but I figured Babel if this is going to require some kind of runtime changes like instrumenting or transforming code in order to cover certain things.

@lelandrichardson
Copy link

Also, If we could actually build this on top of flow, we could have much more insight into the signature of functions, and when they might deopt as a result... Although just writing your JS in strict flow might make it hard to write those deopts in the first place.

@xtuc
Copy link
Member

xtuc commented Jan 9, 2017

I think this make more sense with an eslint plugin which emits warning. This plugin should focus on optimizations and not best practice.

If we focus on v8 performmance, what about other browsers? We might emits to much warnings for optimizations accross all browsers.
A Babel transform plugin makes also sense there since we could just implicitly move stuff.

Aside note: it wouldn't be a surprise to me that v8 would in the future regoranize calls itself as an optimizations. Like other VM does (JVM for example).

@Jessidhia
Copy link
Member

All of the "optimization killers" (except for hash table mode objects) should not be a problem anymore with v8 5.5 anyway.

@xtuc
Copy link
Member

xtuc commented Jan 9, 2017

@Kovensky That's a good news. But there is always legacy stuff sitting around. Could this "optimization killers" also apply to other engines?

@kangax
Copy link
Member

kangax commented Jan 17, 2017

@Kovensky is there some kind of reference to find out more about this — why is it no longer relevant? what exactly was changed and how? etc.

@bgirard
Copy link

bgirard commented Jan 17, 2017

I was actually considering the same thing so I'm glad I found this discussion.

I've written an ESLint warning to match 'delete this.'. This is pretty specific but I think trying to apply this to a large existing code base might produce too many false positives or generally be too noisy and go against developer productivity at the cost of very small performance benefits unless you're in hot code.

I'm leaning that putting this into flow might be the best option. Perhaps letting objects opt into a @Monomorphic annotation.

@xtuc
Copy link
Member

xtuc commented Jan 19, 2017

@bgirard Looks interesting, could you link your repo here?

As a user I wouldn't like having a ton of warning just for the sake of optimization. This should be done implicitly I think. A Babel plugins is actually a better idea IMO.

@hzoo @Kovensky I think this could be a more global subject. As with Babili for minification, what about a Babel project only for engine optimizations.

There are also a lot about parsing performance improvements.

@bgirard
Copy link

bgirard commented Jan 19, 2017

I don't have a public repo accessible but here's a gist:
https://gist.github.com/bgirard/4cd7a2f02f4173a226853bb8b512d09f

It just warns on delete this.<identifier>;. Note that this wont warn on delete this.map[key]; which more likely to be a false positives.

I'm not sure how a babel transformation could make any useful changes statically that would have no observable side-effects. Do you have any ideas?

@hzoo
Copy link
Member Author

hzoo commented Jan 22, 2017

@bgirard You can make a Babel transform just error with something like throw path.buildCodeFrameError("Modules aren't supported in the REPL"); but yeah could just be a linting rule instead. I don't think it's about running the babel in production, more of writing code that will error at runtime or statically to let you know in development?

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

No branches or pull requests

6 participants