clean up sync in async usage #297
Conversation
ghost
assigned 
| if (err) { | ||
| return muxedConn.end(() => { | ||
| callback(err, null) | ||
| }) | ||
| } | ||
|
|
||
| observedAddrs.forEach((oa) => { | ||
| this.switch._peerInfo.multiaddrs.addSafe(oa) | ||
| }) |
There was a problem hiding this comment.
I would recommend using a for loop here instead
src/connection/manager.js
Outdated
| } | ||
| ], (err, peerInfo) => { | ||
| (conn, cb) => identify.dialer(conn, cryptoPI, cb) | ||
| ], (err, peerInfo, observedAddrs) => { |
There was a problem hiding this comment.
You might want to remove that waterfall call, it would improve things as well.
|
I refactored the waterfall logic to use awaits and created some utility methods to make that cleaner. Also removed |
|
With regards to switching forEach loops to for loops etc, I would recommend instead focusing performance improvements on changes that make a difference in terms of big O notation: https://en.wikipedia.org/wiki/Big_O_notation |
|
|
||
| const { peerInfo, observedAddrs } = results | ||
|
|
||
| for (var i = 0; i < observedAddrs.length; i++) { |
There was a problem hiding this comment.
Can you use a for ... of loop here instead?
There was a problem hiding this comment.
for ... of suffers from some performance issues still.
| @@ -0,0 +1,49 @@ | |||
| 'use strict' | |||
There was a problem hiding this comment.
Can you use promisify instead of these functions? https://github.com/digitaldesignlabs/es6-promisify#readme
There was a problem hiding this comment.
I added these as an interim solution. We're going to be doing a lot of work soon for they async iterators initiative libp2p/js-libp2p#266 which will remove the need for these or promisfy. If that ends up taking longer than we'd like, or we have more immediate needs in other parts of the code base, then I think switching over to promisify is warranted.
|
In regards to overall performance improvements, we have an OKR for Q1 to get benchmarking in place for libp2p. That along with the current work for ipfs benchmarking will put us in a better position to do isolated analyses to see where the bottlenecks are in switch and the rest of libp2p. While I think those are very important, I want to avoid pushing more of that into this particular PR, as it's goal was to fix an issue with synchronous code being executed asynchronously, to help alleviate #287. I appreciate all the great feedback! |
I agree, the main gains are in algorithmic improvements in big O notation. However, there is a lot to gain from making the code more optimizable by V8. As an example, This is not always a hot path, but I've see this function in a significant number of flamegraphs. |
This cleans up some logic in the identify flow where sync calls were being done in async code, which can lead to stack bloating. This moves the sync logic out of the async control flow.
Relates to https://github.com/libp2p/js-libp2p-switch/issues/287