-
Notifications
You must be signed in to change notification settings - Fork 52
nextTick instead of setImmediate, and fix sync in async #136
Conversation
Did you try using @mcollina's perf review has pointed to some significant improvements we could make by either eliminating async (seems like that might be a big job here) or using |
The major advantage of Note that the actual issue is caused by having synchronous API that are exposed as async. I think a long-term solution is to refactor those into synchronous calls. |
I switched the setImmediate usage to nextTick everywhere, so that we can use it when it's supported. For the digest this should help quite a bit since that is called during the unboxing of the crypto stream. I have updates to bitswap as well but need to get tests passing there. Bitswap does a lot of network calling and is a big offender of sync in async calls. A lot of the sync code was likely included in the async calls to make control flow easier when it was first built out, but now that we're scaling it's causing problems. I agree that needs to get refactored to just be synchronous where it can be. For things like crypto, where we have interfaces, we'll need to be careful refactoring when the interface needs to by async. Some of our interface implementations are async where others aren't. So we just need to be careful with the interfaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Use https://caolan.github.io/async/docs.html#nextTick so it works in the browser? |
@alanshaw I pushed the change to async/nextTick. The build process should take care of adding checks for process.nextTick, but I made the switch to ensure it's being supported regardless of the build. |
@daviddias or @dignifiedquire can we get a release of this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a box of chocolates for the brave soul that takes on doing a find & replace across all our modules to replace setImmediate to nextTick 🍫
The hmac digest method is made to be async but was calling the callback synchronously. In high throughout scenarios this can cause the call stack to bloat, which in severe instances results in stack overflow issues.
This also fixes linting from the latest aegir to fix ci, and removes non jenkins ci as we're not using travis, circle or appveyor anymore.
Relates to https://github.com/libp2p/js-libp2p-switch/issues/287