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

Low level perf optimizations from Chrome flamegraphs #490

Closed
wants to merge 3 commits into from

Conversation

tazsingh
Copy link
Contributor

@tazsingh tazsingh commented May 25, 2017

This patch solves the following problem
While exploring performance related to #474, these are some issues that I noticed in Chrome's Performance flamegraphs & JavaScript Profiler:

  1. createDOMProps:
    I noticed that Babel was compiling the function with calls to arguments when a default parameter value is used. This dramatically slows down the function execution due to the JavaScript VM not being able to fully optimize the function as it has to check its arguments on every invocation. Changing this to a falsey check avoids this performance pitfall with the same result. This produced a 2x speedup once the function was optimized.
    typeof calls are also generally slow. I've replaced these with calls to constructor which provided an additional 20-30% speedup.

  2. onLayout callback:
    I noticed that the requestAnimationFrame was being scheduled many times during the list's scrolling. This was taking a lot of time to schedule work to be done later. Then once the work itself was being processed, it was doing a tiny amount of work, yielding back to the JS VM to schedule the next amount of tiny work, and so on. This would leave a lot of gaps in time which would cause performance to suffer. Moving this over to a single invocation of requestAnimationFrame to queue up the work to be batched executed later made this about 2-2.5x faster in my not-overly-scientific testing (i.e. difficult to measure accurately due to the variability of the test scenarios).

Test plan
Let me know what you think. I've done some basic testing of VirtualizedList and all works fine. I don't believe anything in this PR has a different result than what was there before and therefore shouldn't introduce any issues.

This pull request

  • includes documentation
  • includes tests
  • includes benchmark reports
  • includes an interactive example
  • includes screenshots/videos

@necolas
Copy link
Owner

necolas commented May 25, 2017

Thanks, sounds promising. I'll test this out today. cc @hzoo for the Babel issue; not the first time I've encountered this issue in RNW or Twitter Lite either

@hzoo
Copy link

hzoo commented May 25, 2017

Not a Babel bug at least, the reason is spec behavior. The reasoning is for function arity I believe, babel/babel#612.

Basically for

function a(b, c = 1) {}
a.length // 1, not 2

Since default parameters don't affect the function arity.

So if we remove the argument in the transpiled code, the only way to get the value is by using arguments.

I guess we could have a loose option for the defaults transform (but since this is non-spec behavior it would need to be an issue/PR if y'all can make that contribution). Wouldn't be too hard to do https://github.com/babel/babel/blob/7.0/packages/babel-plugin-transform-es2015-parameters/src/default.js. Let me know if you need help with contributing.

function a(b, c) {
  if (c === undefined) {
    c = 1;
  }
}

@hzoo
Copy link

hzoo commented May 25, 2017

In the meantime, I would suggest https://github.com/babel/babel-preset-env and maybe looking into multiple bundles such that browsers that support default parameters (or whatever JS) can native support. Although you might find native ES6+ to be slower as well. But simplest thing to do is to add a loose option assuming/understanding that it means you don't care about function length

@tazsingh
Copy link
Contributor Author

Thanks @hzoo for the explanation of this and the reco! I tried out the env preset within the Babel REPL on my browser (Chrome) and noticed that it still compiles the same even though default values are supported by Chrome: link to REPL and link to compat table

Overall I definitely think babel-preset-env would be good to employ for this project. But not sure if it directly applies here as the project does support browsers where the default parameters aren't supported, unless we do different builds for each browser (up to @necolas but may be difficult to distribute and for user understanding).

@hzoo
Copy link

hzoo commented May 25, 2017

screen shot 2017-05-25 at 8 39 29 am

Works fine for me, but yes you would need to figure that out if you want to ship ES6 (and then if you minify figure that out too). Should measure native default parameter perf in browser versions too though. I was thinking about a "perf-table" instead of compat-table but it would be a lot of work that would require community effort.

@tazsingh
Copy link
Contributor Author

Ah yeah looks like if you set the Target to Chrome 58, it compiles appropriately 👍

@necolas
Copy link
Owner

necolas commented May 25, 2017

I'm going to stick with a single build and the browser support required means pretty much everything needs to be converted to ES5. Will probably just avoid default argument values

@hzoo
Copy link

hzoo commented May 26, 2017

Ok just saying we can make a change on Babel's side but that sounds good in the meantime.

@necolas
Copy link
Owner

necolas commented May 26, 2017

Oh I see! Thanks 👍

@tazsingh
Copy link
Contributor Author

tazsingh commented Jun 1, 2017

@necolas Can you let me know if anything further needs to be done here? Would like to get this merged such that we can use it within our apps. Thanks!

@necolas necolas closed this in a9c7b38 Jun 1, 2017
@necolas
Copy link
Owner

necolas commented Jun 1, 2017

Thanks. Available in 0.0.96

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

Successfully merging this pull request may close these issues.

None yet

3 participants