FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: do not chunk strings and Buffer in Readable.from. by mcollina · Pull Request #30912 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (1)ChecksFiles changed
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 commented Dec 12, 2019

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

ronag reviewed Dec 12, 2019
View reviewed changes
Comment thread lib/internal/streams/from.js Outdated

ronag Dec 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina Dec 12, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax commented Dec 12, 2019

Copy link
Copy Markdown
Member

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

mcollina commented Dec 12, 2019

Copy link
Copy Markdown
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 commented Dec 12, 2019

Copy link
Copy Markdown
Member

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

mcollina commented Dec 12, 2019

Copy link
Copy Markdown
Member Author

Ouch. I would consider this a bug :/.

ronag reviewed Dec 12, 2019
View reviewed changes
Comment thread lib/internal/streams/from.js Outdated

ronag Dec 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we check for Stream._isUint8Array as well?

addaleax Dec 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax commented Dec 12, 2019

Copy link
Copy Markdown
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?

mcollina force-pushed the readable-from-fast-strings branch from 378e505 to 019d0d0 Compare December 13, 2019 20:40
mcollina force-pushed the readable-from-fast-strings branch from 019d0d0 to 4dbdd43 Compare December 13, 2019 21:17

nodejs-github-bot commented Dec 14, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27675/

nodejs-github-bot commented Dec 14, 2019

Copy link
Copy Markdown
Collaborator

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

This comment has been minimized.

Sign in to view

Trott commented Dec 14, 2019

Copy link
Copy Markdown
Member

Landed in f8018f2

Trott closed this Dec 14, 2019
Trott pushed a commit that referenced this pull request Dec 14, 2019
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
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 mentioned this pull request Dec 17, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
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 mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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 mentioned this pull request Feb 8, 2020
Merged
davidfirst mentioned this pull request Jul 17, 2020
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ronag ronag approved these changes
Trott Trott approved these changes
addaleax addaleax approved these changes
BridgeAR BridgeAR approved these changes
jasnell Awaiting requested review from jasnell
guybedford Awaiting requested review from guybedford
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL