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

stream: suggestion stream._writeMany #29034

Closed
ronag opened this issue Aug 7, 2019 · 4 comments
Closed

stream: suggestion stream._writeMany #29034

ronag opened this issue Aug 7, 2019 · 4 comments
Labels
feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Aug 7, 2019

Currently the stream.writev API has two drawbacks:

  • It requires adding properties to the input array (allBuffers).
  • It will always creates an array copy.
  • The array entries must be allocated objects.

It would be nice if user space could just create the correctly formatted array without having to perform any allocations, copies and transformations.

In order to not break anything I suggest a new signature writev maybe called writeMany?

Which would look something like:

Stream._writeMany = function (chunks, allBuffers, cb) {
}

e.g.

const chunks = [];
for (const { chunk } of data) {
  chunks.push(chunk);
}
w._writeMany(chunks, true, cb);
const chunks = [];
for (const { chunk, encoding } of data) {
  chunks.push(chunk, encoding);
}
w._writeMany(chunks, false, cb);

Looking at the current Writable implementation, the writev scenario will always create two "unnecessary" array copies in order to pass the chunks. Furthermore all the array entries are allocated objects.

@ronag ronag changed the title stream: suggestion writev2 stream: suggestion stream._writeMany Aug 7, 2019
@mscdex
Copy link
Contributor

mscdex commented Aug 7, 2019

allBuffers is an implementation detail and not something that should be requested from the end user and relied upon.

@ronag
Copy link
Member Author

ronag commented Aug 7, 2019

allBuffers is an implementation detail and not something that should be requested from the end user and relied upon

True, but by exposing it we enable some optimizations that are otherwise not possible.

@mscdex
Copy link
Contributor

mscdex commented Aug 7, 2019

We would still need to validate that the value for allBuffers is correct, which would negate any gain.

@ronag
Copy link
Member Author

ronag commented Aug 7, 2019

We would still need to validate that the value for allBuffers is correct, which would negate any gain.

I'm not sure I follow?

@Fishrock123 Fishrock123 added stream Issues and PRs related to the stream subsystem. feature request Issues that request new features to be added to Node.js. labels Aug 7, 2019
@ronag ronag closed this as completed Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants