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

No longer use global setImmediate function due to performance issues #335

Merged
merged 1 commit into from
Jan 12, 2018

Conversation

robframpton
Copy link

@robframpton robframpton commented Jan 10, 2018

async.nextTick

Metal core currently provides an async module which has been brought over from google closure library, one of the most used methods from that module is async.nextTick. This method makes use of the browser’s setImmediate function if it is defined (currently only IE10+/Edge have a native implementation).

Because most browsers do not have setImmediate defined, the async module provides a fallback for this functionality. In addition, commonly used babel presets will automatically provide a polyfill from core-js. If the polyfill is defined, it will win over the fallback.

The problem is that the polyfill from core-js is significantly slower than the fallback provided by google closure. In addition, the setImmediate implementation in IE has known issues. This means the only reliable native implementation is in Edge.

Due to these issues, closure library has introduced a change to their nextTick function so that it only uses the globally defined setImmediate function in Edge, or if the global function is an instance of their MockClock utility (which we do not provide in Metal).

My proposal is to never use the globally defined setImmediate method in Metal, and to always use the fallback provided by async. That way we know we are using an optimized solution, and it is consistent across all browsers and environments.

Metrics

My test case consisted of a table with 10,000 rows, and another with just 1000 rows. Each row being a sub component of the parent.

In both scenarios, the time it took to update the rows with new data was reduced by ~48% by no longer using the polyfill on Chrome.

10,000 row update with setImmediate polyfill

screen shot 2018-01-10 at 12 13 50 pm

10,000 row update with closure library's fallback

screen shot 2018-01-10 at 12 16 50 pm

@mthadley
Copy link

This is really awesome!

@brunobasto
Copy link

Awesome, @Robert-Frampton!!! Great job!

Copy link
Contributor

@jbalsas jbalsas left a comment

Choose a reason for hiding this comment

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

Hey @Robert-Frampton, great job! I have 2 things to run by you:

  1. Is the perf test coming from https://github.com/Robert-Frampton/js-framework-benchmark/tree/metal? Is there a way we could stablish some baselines for those in this repo so we can run prior to releases to make sure we don't regress back to bad performance numbers?

  2. Has this been tested in IE9-11? Based on Closure Library's::nexttick code for IE they always opt-out of the Channel fallback as well due to the problems you mentioned (with IE11 falling back to setTimeout(fn,0)), but in our case, it looks like we might likely still hit the low perf fallback...

@robframpton
Copy link
Author

@jbalsas

I've tested with IE11, and it seems to be working faster than it was before. I also tried with forcing it to use setTimeout(fn,0) which caused it to be slower than it is now.

@jbalsas
Copy link
Contributor

jbalsas commented Jan 12, 2018

Thanks for checking @Robert-Frampton! Merging it then! 🎉

@jbalsas jbalsas merged commit ac2ecac into metal:develop Jan 12, 2018
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.

4 participants