Skip to content

stream: do not chunk strings and Buffer in Readable.from.#30912

Closed
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:readable-from-fast-strings
Closed

stream: do not chunk strings and Buffer in Readable.from.#30912
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:readable-from-fast-strings

Conversation

@mcollina
Copy link
Copy Markdown
Member

@mcollina mcollina commented Dec 12, 2019

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
ronag
ronag reviewed Dec 12, 2019
View reviewed changes
Comment thread lib/internal/streams/from.js Outdated
Copy link
Copy Markdown
Member

@ronag ronag Dec 12, 2019 •
edited
Loading

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
Copy Markdown
Member Author

@mcollina mcollina Dec 12, 2019

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
Copy Markdown
Member

addaleax commented Dec 12, 2019

This does change behaviour when a buffer is passed, is that intentional?
@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Dec 12, 2019

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
Copy Markdown
Member

targos commented Dec 12, 2019

@mcollina yes, but instead of emitting numbers, it now emits a Buffer
@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Dec 12, 2019

Ouch. I would consider this a bug :/.
ronag
ronag reviewed Dec 12, 2019
View reviewed changes
Comment thread lib/internal/streams/from.js Outdated
Copy link
Copy Markdown
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.

Should we check for Stream._isUint8Array as well?
Copy link
Copy Markdown
Member

@addaleax addaleax Dec 12, 2019

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
Copy Markdown
Member

addaleax commented Dec 12, 2019

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?
@mcollina mcollina force-pushed the readable-from-fast-strings branch from 378e505 to 019d0d0 Compare December 13, 2019 20:40
@mcollina mcollina force-pushed the readable-from-fast-strings branch from 019d0d0 to 4dbdd43 Compare December 13, 2019 21:17
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Dec 14, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27675/
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Dec 14, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/27685/
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott
Copy link
Copy Markdown
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
@mcollina @Trott
stream: do not chunk strings and Buffer in Readable.from
f8018f2 
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@mcollina @MylesBorins
stream: do not chunk strings and Buffer in Readable.from
0c18c49 
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@mcollina @targos
stream: do not chunk strings and Buffer in Readable.from
8cd8cd7 
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@mcollina @BethGriggs
stream: do not chunk strings and Buffer in Readable.from
322912a 
PR-URL: #30912
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
@davidfirst davidfirst mentioned this pull request Jul 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

7 participants

@mcollina @addaleax @targos @nodejs-github-bot @Trott @ronag @BridgeAR