Skip to content

stream: support typed arrays - #51866

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
IlyasShabi:streams-typed-array
Mar 20, 2024
Merged

stream: support typed arrays#51866
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
IlyasShabi:streams-typed-array

Conversation

@IlyasShabi

@IlyasShabiIlyasShabi commented Feb 24, 2024

Copy link
Copy Markdown
Member

This PR aims to add support for typed arrays in streams, addressing this issue #51728
cc: @nodejs/streams

@nodejs-github-bot

nodejs-github-bot commented Feb 24, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Feb 24, 2024
mcollina
mcollina reviewed Feb 25, 2024
View reviewed changes

@mcollinamcollina left a comment

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.

Can you also update the docs?

Comment threadlib/stream.js Outdated
@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024
@nodejs-github-bot

nodejs-github-bot commented Feb 25, 2024

Copy link
Copy Markdown
Collaborator

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

@IlyasShabi
IlyasShabiforce-pushed the streams-typed-array branch 2 times, most recently from 9a4bb10 to 16dddd2CompareFebruary 26, 2024 15:16
@IlyasShabi

IlyasShabi commented Feb 26, 2024

Copy link
Copy Markdown
MemberAuthor

@mcollina could you please check again ?

@marco-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@nodejs-github-bot

nodejs-github-bot commented Feb 27, 2024

Copy link
Copy Markdown
Collaborator

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

mcollina
mcollina approved these changes Feb 27, 2024
View reviewed changes

@mcollinamcollina left a comment

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.

lgtm

rluvaton
rluvaton reviewed Feb 27, 2024
View reviewed changes

@rluvatonrluvaton left a comment

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.

Thank you for your contribution, left minor comment

Comment threadlib/internal/streams/readable.js Outdated
Comment threadlib/internal/streams/writable.js Outdated
@IlyasShabi
IlyasShabiforce-pushed the streams-typed-array branch 2 times, most recently from 76463b6 to 3fb394fCompareFebruary 27, 2024 20:49
addaleax
addaleax reviewed Feb 27, 2024
View reviewed changes
Comment threaddoc/api/stream.md Outdated

@addaleaxaddaleaxFeb 27, 2024

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.

You'll probably want to write down what writing a TypedArray actually does, because there are at least two obvious potential options (values or underlying bytes) and the docs currently don't say which it is. :)

ShogunPanda
ShogunPanda approved these changes Feb 28, 2024
View reviewed changes

@ShogunPandaShogunPanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

benjamingr
benjamingr requested changes Mar 1, 2024
View reviewed changes

@benjamingrbenjamingr left a comment

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.

Generally LGTM, please address Anna's comment and run benchmarks to see if this causes a regression in Uint8Array usage (let me know if you need instructions on how to do those) :)

@IlyasShabi

IlyasShabi commented Mar 2, 2024

Copy link
Copy Markdown
MemberAuthor

I have made some updates to the PR. Could you please review it again? 🙏
@addaleax@benjamingr I also changed the doc and added benchmarks

@IlyasShabi

IlyasShabi commented Mar 2, 2024

Copy link
Copy Markdown
MemberAuthor

local benchmark results:

 confidence improvement accuracy (*) (**) (***)
streams/readable-uint8array.js kind='encoding' n=1000000 -1.45 % ±2.62% ±3.49% ±4.54%
streams/readable-uint8array.js kind='read' n=1000000 -0.70 % ±2.46% ±3.28% ±4.27%
streams/writable-uint8array.js kind='object-mode' n=50000000 -0.36 % ±0.67% ±0.90% ±1.17%
streams/writable-uint8array.js kind='write' n=50000000 1.18 % ±1.27% ±1.70% ±2.22%
streams/writable-uint8array.js kind='writev' n=50000000 * 1.26 % ±1.21% ±1.61% ±2.10%

@mcollinamcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 20, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 20, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 7d258db into nodejs:mainMar 20, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 20, 2024

Copy link
Copy Markdown
Collaborator

Landed in 7d258db

@mcollinamcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 21, 2024
@github-actions

github-actionsBot commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

The notable-changePRs with changes that should be highlighted in changelogs. label has been added by @mcollina.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@mcollinamcollina added the baking-for-lts PRs that need to wait before landing in a LTS release. label Mar 21, 2024
@mcollina

mcollina commented Mar 21, 2024

Copy link
Copy Markdown
Member

Note that this broke undici extensive test suite. Out of caution, I'm going to add the backing--for-lts label

@mcollinamcollina added semver-minor PRs that contain new features and should be released in the next minor version. and removed needs-ci PRs that need a full CI run. labels Mar 21, 2024
@RafaelGSSRafaelGSS mentioned this pull request Apr 15, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@IlyasShabi@marco-ippolito
stream: support typed arrays
c31ed19
PR-URL: #51866
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@marco-ippolitomarco-ippolito mentioned this pull request May 2, 2024
Merged
marco-ippolito added a commit that referenced this pull request May 2, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
2e68800
Notable changes:
benchmark:
* add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
* improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
* deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
* (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
* (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
* (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
* (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
* update release gpg keyserver (marco-ippolito) #52257
* add release key for marco-ippolito (marco-ippolito) #52257
* add UlisesGascon as a collaborator (Ulises Gascón) #51991
* (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
* mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
* add stacktrace to fs/promises (翠 / green) #49849
lib, url:
* (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
* (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
* (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
* (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
* (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
* (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
* (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
* (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
* (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
* (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038
* (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
* (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
* (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
* mark as stable (Moshe Atlow) #52074
PR-URL: #52793
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@IlyasShabi@marco-ippolito
stream: support typed arrays
0b853a7
PR-URL: #51866
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito added a commit that referenced this pull request May 3, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
1766b1d
Notable changes:
benchmark:
* add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
* improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
* deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
* (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
* (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
* (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
* (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
* update release gpg keyserver (marco-ippolito) #52257
* add release key for marco-ippolito (marco-ippolito) #52257
* add UlisesGascon as a collaborator (Ulises Gascón) #51991
* (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
* mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
* add stacktrace to fs/promises (翠 / green) #49849
lib, url:
* (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
* (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
* (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
* (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
* (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
* (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
* (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
* (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
* (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
* (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
* (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
* (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
* mark as stable (Moshe Atlow) #52074
PR-URL: #52793
marco-ippolito added a commit that referenced this pull request May 7, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
0d0efa4
Notable changes:
benchmark:
* add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
* improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
* deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
* (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
* (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
* (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
* (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
* update release gpg keyserver (marco-ippolito) #52257
* add release key for marco-ippolito (marco-ippolito) #52257
* add UlisesGascon as a collaborator (Ulises Gascón) #51991
* (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
* mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
* add stacktrace to fs/promises (翠 / green) #49849
lib, url:
* (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
* (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
* (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
* (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
* (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
* (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
* (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
* (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
* (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
* (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
* (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
* (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
* mark as stable (Moshe Atlow) #52074
PR-URL: #52793
marco-ippolito added a commit that referenced this pull request May 7, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
9a1df15
Notable changes:
benchmark:
* add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
* improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
* deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
* (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
* (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
* (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
* (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
* update release gpg keyserver (marco-ippolito) #52257
* add release key for marco-ippolito (marco-ippolito) #52257
* add UlisesGascon as a collaborator (Ulises Gascón) #51991
* (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
* mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
* add stacktrace to fs/promises (翠 / green) #49849
lib, url:
* (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
* (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
* (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
* (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
* (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
* (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
* (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
* (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
* (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
* (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
* (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
* (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
* mark as stable (Moshe Atlow) #52074
PR-URL: #52793
@targos

targos commented May 8, 2024

Copy link
Copy Markdown
Member

@mcollina This landed on v20.13.0 by mistake. Should we revert it?

@mcollina

mcollina commented May 8, 2024

Copy link
Copy Markdown
Member

I would not revert now, but wait if there are bugs coming out of it. If there are, we revert.

(If you are assembling a revert release it might be good to be cautious, but don’t do one for this).

@marco-ippolitomarco-ippolito removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Jun 17, 2024
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@marco-ippolito@soophoo
2024-05-07, Version 20.13.0 'Iron' (LTS)
66ad117
Notable changes:
benchmark:
* add AbortSignal.abort benchmarks (Raz Luvaton) nodejs#52408
buffer:
* improve `base64` and `base64url` performance (Yagiz Nizipli) nodejs#52428
crypto:
* deprecate implicitly shortened GCM tags (Tobias Nießen) nodejs#52345
deps:
* (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) nodejs#52138
* (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) nodejs#51462
* (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) nodejs#51278
dns:
* (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) nodejs#52492
doc:
* update release gpg keyserver (marco-ippolito) nodejs#52257
* add release key for marco-ippolito (marco-ippolito) nodejs#52257
* add UlisesGascon as a collaborator (Ulises Gascón) nodejs#51991
* (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) nodejs#51879
events,doc:
* mark CustomEvent as stable (Daeyeon Jeong) nodejs#52618
fs:
* add stacktrace to fs/promises (翠 / green) nodejs#49849
lib, url:
* (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) nodejs#52509
net:
* (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) nodejs#52474
report:
* (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) nodejs#51645
src:
* (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) nodejs#52595
* (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) nodejs#51977
* (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) nodejs#52023
* (SEMVER-MINOR) preload function for Environment (Cheng Zhao) nodejs#51539
stream:
* (SEMVER-MINOR) support typed arrays (IlyasShabi) nodejs#51866
test_runner:
* (SEMVER-MINOR) add suite() (Colin Ihrig) nodejs#52127
* (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) nodejs#51909
util:
* (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) nodejs#52040
v8:
* (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) nodejs#51927
watch:
* mark as stable (Moshe Atlow) nodejs#52074
PR-URL: nodejs#52793
@IlyasShabi
IlyasShabi deleted the streams-typed-array branch October 16, 2024 18:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax left review comments
@mcollinamcollinamcollina approved these changes
@ShogunPandaShogunPandaShogunPanda approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@rluvatonrluvatonrluvaton approved these changes
@ronagronagAwaiting requested review from ronag

Assignees

No one assigned

Labels

notable-changePRs with changes that should be highlighted in changelogs.semver-minorPRs that contain new features and should be released in the next minor version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@IlyasShabi@nodejs-github-bot@mcollina@targos@ShogunPanda@addaleax@benjamingr@rluvaton@marco-ippolito@ilyasShabiCS