Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Avoid multiple conversions between node streams and pull streams #89

Closed
mcollina opened this issue Jan 11, 2019 · 2 comments
Closed

Avoid multiple conversions between node streams and pull streams #89

mcollina opened this issue Jan 11, 2019 · 2 comments
Assignees
Labels
exp/wizard Extensive knowledge (implications, ramifications) required help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature P0 Critical: Tackled by core team ASAP status/in-progress In progress

Comments

@mcollina
Copy link

This module converts between streams and pull streams several times, increasing memory usage, latency and cpu usage on the hot path of receiving data.

The mplex.listener(stream) method require a pull stream. However, that pull stream is automatically converted into a node core stream in

const stream = toStream(rawConn)
.
This is going to lead to adding two layers to abstractions that are not needed on Node.js, as mplex core multiplexer is implemented as a Node.js duplex (
class Multiplex extends stream.Duplex {
).

My recommendation is to move the internal multiplexer to a pull-stream only implementation, because that will also remove the conversion needed when emitting a new stream (

catchError(toPull.duplex(stream)),
). In turn, this create even more overhead.

Overall, this module introduces 5/6 levels of data buffering, where 1 or 2 could suffice (each layer introduce latency. As a general rule, every time you pipe two streams it adds some data buffering and latency.


When doing this port to pull-stream, I recommend to remove one of the main buffering layer. Currently the module creates a Duplex stream which is piped back and fort (

pump(stream, mpx, stream)
). Note that this duplex is only internal, and it is not exposed, so it's totally unnecessary. It's possible to use the native node core or pull stream methods to read and write to it, saving a buffering layer.


Another element of latency is introduced by the usage of duplexify, which adds one unneeded level of buffering. Duplexify is used to assemble shared streams in

return duplexify(this.createStream(name, Object.assign(opts, {lazy: true})), this.receiveStream(name, opts))
. Even if it's convenient, creating dupexes in this way is inefficient, and they should but created as duplex from the start.

@vasco-santos vasco-santos added kind/enhancement A net-new feature or improvement to an existing feature exp/wizard Extensive knowledge (implications, ramifications) required status/ready Ready to be worked P0 Critical: Tackled by core team ASAP help wanted Seeking public contribution on this issue labels Jan 11, 2019
@jacobheun jacobheun self-assigned this Feb 4, 2019
@jacobheun jacobheun added status/in-progress In progress and removed status/ready Ready to be worked labels Feb 4, 2019
@daviddias
Copy link
Member

All shall be gone with #94

@vasco-santos
Copy link
Member

It's done 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/wizard Extensive knowledge (implications, ramifications) required help wanted Seeking public contribution on this issue kind/enhancement A net-new feature or improvement to an existing feature P0 Critical: Tackled by core team ASAP status/in-progress In progress
Projects
None yet
Development

No branches or pull requests

4 participants