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

Use async/setImmediate instead of async/nextTick #196

Closed
satazor opened this issue Jun 14, 2019 · 5 comments
Closed

Use async/setImmediate instead of async/nextTick #196

satazor opened this issue Jun 14, 2019 · 5 comments

Comments

@satazor
Copy link

satazor commented Jun 14, 2019

I've noticed that this module is using async/nextTick instead of async/setImmediate. Using async/setImmediate is preferred due to the way async/nextTick works in the browser:

Calls callback on a later loop around the event loop. In Node.js this just calls process.nextTick. In the browser it will use setImmediate if available, otherwise setTimeout(callback, 0), which means other higher priority events may precede the execution of callback.

Because timers are often throttled when a tab is not active to improve battery life, they only run at 1Hz (once every second). This has a drastic performance impact in those scenarios.

Will you accept a PR to change this?

Meanwhile, I'm aliasing async/nextTick to async/setImmediate in my webpack config.

@dirkmc
Copy link
Contributor

dirkmc commented Jun 14, 2019

Sure, please go ahead and make a PR. Thank you

@satazor
Copy link
Author

satazor commented Jun 14, 2019

Note that even if I change to async/setImmediate there's no guarantee that setImmediate will be available. In fact, in most browsers it doesn't. The thing is that webpack seems to automatically inject/polyfill setImmediate by default.

I will make a PR.

@achingbrain
Copy link
Member

Please use nextTick over setImmediate. The reason is because callbacks queued up by process.nextTick are executed at the end of the current tick, ones queued by setImmediate are executed at the start of the next tick (confusingly) which introduces a small amount of latency. On hot codepaths this mounts up.

The NearForm team did a whole bunch of performance analysis of our code base at the beginning of the year and this was one of their findings - their recommendation was to avoid creating latency like this wherever possible.

@satazor
Copy link
Author

satazor commented Jun 14, 2019

@achingbrain I understand. For me this is actually a webpack problem because its process.nextTick polyfill relies on setTimeout, which run once each 1sec if tab is not focused/active.

The sad reality is that most people using IPFS in the browser will experience the slowness I was experiencing while webpack doesn't fix their polyfill. I had the will to pinpoint the problem and it took me 3 hours, but most people won't and will just give up.

Feel free to close #197. I've opened an issue on Webpack: webpack/node-libs-browser#92

I will keep the aliasing on my part to get around the issue.

@dirkmc
Copy link
Contributor

dirkmc commented Jun 14, 2019

Ok thanks @achingbrain for your input, I didn't know that. I will go ahead and close this issue and the PR as it sounds like the fix should be made in Webpack and @satazor has opened an issue with them.

@dirkmc dirkmc closed this as completed Jun 14, 2019
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 a pull request may close this issue.

3 participants