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: do not chunk strings and Buffer in Readable.from. #30912

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

This PR makes Readable.from() to not iterate over strings and buffers to avoid unnecessary overhead.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines


const {
ERR_INVALID_ARG_TYPE
} = require('internal/errors').codes;

function from(Readable, iterable, opts) {
let iterator;
if (typeof iterable === 'string' || iterable instanceof Buffer) {
return new Readable({
objectMode: true,
Copy link
Member

@ronag ronag Dec 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, Not sure of the purpose here, but wouldn't objectMode: false make more sense here as a default? Or is that breaking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep it consistent with the rest. Also, it would change the encoding, so possibly it's not a good idea.

@addaleax
Copy link
Member

This does change behaviour when a buffer is passed, is that intentional?

@mcollina
Copy link
Member Author

This does change behaviour when a buffer is passed, is that intentional?

Yes, instead of emitting one byte at a time, we will push through the full chunk. This will remove a lot of overhead when using this API.

@targos
Copy link
Member

targos commented Dec 12, 2019

@mcollina yes, but instead of emitting numbers, it now emits a Buffer

@mcollina
Copy link
Member Author

Ouch. I would consider this a bug :/.


const {
ERR_INVALID_ARG_TYPE
} = require('internal/errors').codes;

function from(Readable, iterable, opts) {
let iterator;
if (typeof iterable === 'string' || iterable instanceof Buffer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check for Stream._isUint8Array as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would only be okay if we turn off object mode, I think

@addaleax
Copy link
Member

Ouch. I would consider this a bug :/.

I would agree, but could you document that Buffers are treated differently from other iterables in that case?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member

Trott commented Dec 14, 2019

Landed in f8018f2

@Trott Trott closed this Dec 14, 2019
Trott pushed a commit that referenced this pull request Dec 14, 2019
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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

Successfully merging this pull request may close these issues.

7 participants