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

Duplex seems to create unnecessary copies of Buffer data #162

Closed
jamestalmage opened this issue Oct 9, 2015 · 6 comments
Closed

Duplex seems to create unnecessary copies of Buffer data #162

jamestalmage opened this issue Oct 9, 2015 · 6 comments

Comments

@jamestalmage
Copy link

The assertion below fails in iojs >= 3.0.0, and Node >= 4.0.0. It passes in every earlier version I checked (0.8.28 to 2.5.0).

var stream = require('readable-stream');

var duplex = new stream.Duplex({
    write: function(chunk, enc, cb) {
        this.push(chunk);
        cb();
    },
    read: function() {}
});

var bufA = new Buffer('a');

var writable = new stream.Writable({
    write: function (chunk, enc, cb) {
        assert.strictEqual(chunk, bufA, 'should be the same instance');
    }
});

duplex.pipe(writable);

duplex.write(bufA);

It can be made to pass by setting readableObjectMode: true, but it is unclear to me why that would be necessary (or the implications it would have).

@yoshuawuyts
Copy link

@jamestalmage did you check if this is also the case for readable streams?

@calvinmetcalf
Copy link
Contributor

there is no guarantee of buffer equality for streams not in object mode, the buffers can be combined and they can be split. If it's just the possible perf issues then we should look to see where the copy is happening.

@jamestalmage
Copy link
Author

This does not happen with readable streams.

My main concern here is the performance impact.

objectMode:true seems undesirable since it seems it will have implications on buffering/backpressure.

there is no guarantee of buffer equality for streams not in object mode, the buffers can be combined and they can be split

Just for my understanding: Why is that ever necessary in the base streams implementation? It seems to me that the goal would be to never copy data unnecessarily, and I am having a hard time thinking of a reason why a base stream implementation would ever need to.

@calvinmetcalf
Copy link
Contributor

figured out the reason for this nodejs/node#3300

@jamestalmage
Copy link
Author

This does not happen with readable streams.

Oops! I obviously recalled incorrectly (probably got confused with my testing on node 12.X). Sorry. And thanks for the fix!

@calvinmetcalf
Copy link
Contributor

fixed and is in both node and readable-streams

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

No branches or pull requests

3 participants