-
Notifications
You must be signed in to change notification settings - Fork 62
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
Speed up nextTick in web workers #25
Conversation
I made a perftest and it seems that for Chrome, |
@jussi-kalliokoski you are creating a new object and calling Object.observe on every test run. If you do this once in the setup phase and just increment a property on each run, Object.observe is faster. Did you see the jsperf I linked to before? http://jsperf.com/setimmediate-test/17 |
@defunctzombie any thoughts on this? |
@devongovett whoops, sorry! Completely missed the last paragraph. 👍 (Nice test case, btw!) |
I am concerned that the size of this library keeps growing. It is better if On Monday, November 24, 2014, Jussi Kalliokoski notifications@github.com
|
Yeah but the problem is that the other core modules (e.g. streams) use |
@devongovett fair, ok will take a closer look over next few days |
There is a bug in IE that causes message chanel (and post message and setInmediate see ) to be broken cujojs/when#197 you need to make sure IE never gets here and we never get here in the window. Your perf issues are more likely due to the use of unshift in the queue not the latency of calling the async function as that is only needed the first time. |
The perf issues are definitely not the queuing. As mentioned above, inside workers none of the existing async functions used are available so setTimeout is being used. In the jsperf comparing the techniques, the same queuing behavior is used for all of the tests, so that can't be the issue. |
I meant the performance issue in the app, in the perf you are just measuring latency of the async providing method with a queue the actual async method is less relevent |
I have merged another PR (62acf3e) which should help with perf and is a simpler fix. Please test out master with this merge and let me know if things are better. If they are, then I will release a version with these changes. |
Yes, that is definitely a good fix. Seems similar in performance to me, for my usage. Would be nice to at least use |
sadly not so simple, the version in IE is broken cujojs/when#197 and certain things can prevent setImmediate (and MessageChannel and postMessage) from ever returning. If you want low latency next tick I've written a dedicated module for that which handles all the ridiculous corner cases. Bear in mind the problem you are trying to solve is latency not performance, if you have multiple images being processed at the same time in the same worker then the latency will likely end up not being noticeable as the thread won't be wasting time when idle. |
I've been working on some streaming image processing, and I was debugging why it was running 10 times slower in web workers than on the main thread. I did some profiling and saw lots of what looked like idle time. After a few days I figured it out: the node stream module calls process.nextTick a lot and it was falling back to
setTimeout
inside web workers since mutation observers and postMessage are not supported there.This PR adds three async yielding methods that are supported by workers in various browsers, in order of speed according to http://jsperf.com/setimmediate-test/17. They are:
Object.observe
(fastest by far in Chrome), nativePromise
(fastest in Firefox and new Safari), andMessageChannel
(old Chrome and Safari).