stream: readable bimap by ronag · Pull Request #49963 · nodejs/node

ronag · 2023-09-29T17:42:32Z

Still not complete but another step in the right direction.

nodejs-github-bot · 2023-09-29T17:42:36Z

Review requested:

@nodejs/streams

ronag · 2023-09-29T19:28:01Z

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1430/

mscdex · 2023-09-29T23:41:08Z

s/bimap/bitmap/ in commit message

ronag · 2023-09-30T02:43:12Z

16:59:22 streams/creation.js kind='duplex' n=50000000                                                    ***     12.35 %       ±1.40%  ±1.88%  ±2.48%
16:59:22 streams/creation.js kind='readable' n=50000000                                                  ***     -4.51 %       ±0.79%  ±1.06%  ±1.39%
16:59:22 streams/creation.js kind='transform' n=50000000                                                 ***     12.76 %       ±1.82%  ±2.42%  ±3.16%
16:59:22 streams/creation.js kind='writable' n=50000000                                                          -0.64 %       ±1.02%  ±1.35%  ±1.76%
16:59:22 streams/destroy.js kind='duplex' n=1000000                                                       **     11.80 %       ±7.99% ±10.63% ±13.83%
16:59:22 streams/destroy.js kind='readable' n=1000000                                                             2.41 %       ±2.57%  ±3.43%  ±4.47%
16:59:22 streams/destroy.js kind='transform' n=1000000                                                   ***     15.18 %       ±6.19%  ±8.24% ±10.74%
16:59:22 streams/destroy.js kind='writable' n=1000000                                                             1.10 %       ±2.65%  ±3.53%  ±4.60%
16:59:22 streams/pipe-object-mode.js n=5000000                                                                   -2.38 %       ±3.85%  ±5.14%  ±6.70%
16:59:22 streams/pipe.js n=5000000                                                                       ***      7.06 %       ±3.17%  ±4.21%  ±5.48%
16:59:22 streams/readable-async-iterator.js sync='no' n=100000                                            **      4.96 %       ±2.83%  ±3.79%  ±4.98%
16:59:22 streams/readable-async-iterator.js sync='yes' n=100000                                                  -1.89 %       ±4.03%  ±5.42%  ±7.18%
16:59:22 streams/readable-bigread.js n=1000                                                                       1.83 %       ±2.07%  ±2.75%  ±3.59%
16:59:22 streams/readable-bigunevenread.js n=1000                                                                 1.53 %       ±6.04%  ±8.04% ±10.46%
16:59:22 streams/readable-boundaryread.js type='buffer' n=2000                                           ***      4.27 %       ±1.37%  ±1.83%  ±2.41%
16:59:22 streams/readable-boundaryread.js type='string' n=2000                                           ***      3.12 %       ±1.50%  ±1.99%  ±2.60%
16:59:22 streams/readable-from.js n=10000000                                                             ***     11.41 %       ±2.08%  ±2.76%  ±3.60%
16:59:22 streams/readable-readall.js n=5000                                                                       0.34 %       ±0.70%  ±0.93%  ±1.21%
16:59:22 streams/readable-unevenread.js n=1000                                                           ***      3.39 %       ±0.46%  ±0.61%  ±0.80%
16:59:22 streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000                    -0.68 %       ±2.02%  ±2.69%  ±3.53%
16:59:22 streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000                    2.53 %       ±3.57%  ±4.75%  ±6.19%
16:59:22 streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000                    0.28 %       ±2.45%  ±3.28%  ±4.31%
16:59:22 streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000                   0.67 %       ±2.94%  ±3.91%  ±5.09%
16:59:22 streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000                   -1.20 %       ±2.37%  ±3.17%  ±4.16%
16:59:22 streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000                   1.15 %       ±2.52%  ±3.36%  ±4.38%
16:59:22 streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000                   0.01 %       ±2.74%  ±3.66%  ±4.80%
16:59:22 streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000                  0.36 %       ±2.58%  ±3.44%  ±4.47%
16:59:22 streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000                   -0.28 %       ±1.15%  ±1.53%  ±1.99%
16:59:22 streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000                   1.28 %       ±1.92%  ±2.56%  ±3.35%
16:59:22 streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000                  -0.03 %       ±1.44%  ±1.92%  ±2.50%
16:59:22 streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000                  1.88 %       ±1.92%  ±2.57%  ±3.37%
16:59:22 streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000                  -0.55 %       ±1.28%  ±1.72%  ±2.24%
16:59:22 streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000                 -0.48 %       ±0.94%  ±1.25%  ±1.63%
16:59:22 streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000                  0.65 %       ±1.32%  ±1.75%  ±2.29%
16:59:22 streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000         **     -1.84 %       ±1.19%  ±1.58%  ±2.07%

nodejs-github-bot · 2023-09-30T02:46:48Z

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

benjamingr

I had a branch that does some of this (which you saw on our video call) but haven't been in office since (it's Sukkot here and we're on holiday). Anyway I'll review mostly should be good/similar.

benjamingr

This has more work than my branch so probably let's keep it on this one - there are a few places missing conversion to bitmap (also in the old properties) IIRC I'll check when I'm able

lib/internal/streams/readable.js

PR-URL: nodejs#49963

nodejs-github-bot · 2023-10-16T22:15:48Z

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

lib/internal/streams/readable.js

nodejs-github-bot · 2023-10-17T06:50:18Z

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

nodejs-github-bot · 2023-10-17T09:32:14Z

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

tniessen · 2023-10-17T12:31:23Z

Nit: commit message does not adhere to guidelines.

nodejs-github-bot · 2023-10-17T14:08:34Z

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

tniessen · 2023-10-17T14:29:30Z

(Removed the author-ready label due to the commit message. Please amend before merging.)

Uzlopak · 2023-10-17T14:30:34Z

Should it be streams: readable bitmap or what do you suggest?

tniessen · 2023-10-17T14:33:33Z

Please refer to the commit message guidelines. The commit message must begin with the subsystem prefix (e.g., stream:) followed by a an imperative verb in lower case.

Trott · 2023-10-17T17:18:36Z

Should it be something like stream: reduce scope of readable bitmap details? I'm happy to manually land this and amend the commit message, but I want to make sure I get it right and also keep the first line reasonably concise.

lpinca

RSLGTM

Trott

Rubber-stamp

Trott · 2023-10-17T19:10:09Z

Landed in 4e70d23

PR-URL: #49963 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#49963 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #49963 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

ronag requested review from benjamingr and mcollina September 29, 2023 17:42

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 29, 2023

ronag force-pushed the readable-bimap2 branch 14 times, most recently from 276dfec to 30ad6c0 Compare September 29, 2023 19:25

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023

benjamingr reviewed Sep 30, 2023

View reviewed changes

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

benjamingr reviewed Sep 30, 2023

View reviewed changes

lib/internal/streams/readable.js Show resolved Hide resolved

benjamingr reviewed Sep 30, 2023

View reviewed changes

lib/internal/streams/readable.js Show resolved Hide resolved

github-actions bot mentioned this pull request Oct 1, 2023

CI Reliability 2023-10-01 nodejs/reliability#680

Open

16 tasks

rluvaton reviewed Oct 16, 2023

View reviewed changes

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

rluvaton reviewed Oct 16, 2023

View reviewed changes

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

ronag commented Oct 16, 2023

View reviewed changes

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

stream: readable bimap

28b85ad

PR-URL: nodejs#49963

ronag force-pushed the readable-bimap2 branch from c691e56 to 28b85ad Compare October 16, 2023 22:12

ronag requested a review from rluvaton October 16, 2023 22:13

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2023

rluvaton reviewed Oct 16, 2023

View reviewed changes

lib/internal/streams/readable.js Show resolved Hide resolved

rluvaton approved these changes Oct 17, 2023

View reviewed changes

tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 17, 2023

lpinca approved these changes Oct 17, 2023

View reviewed changes

Trott approved these changes Oct 17, 2023

View reviewed changes

Trott merged commit 4e70d23 into nodejs:main Oct 17, 2023

targos mentioned this pull request Oct 23, 2023

v21.1.0 release proposal #50335

Merged

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

Uh oh!

Conversation

ronag commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 29, 2023

Uh oh!

ronag commented Sep 29, 2023

Uh oh!

mscdex commented Sep 29, 2023

Uh oh!

ronag commented Sep 30, 2023

Uh oh!

nodejs-github-bot commented Sep 30, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 16, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 17, 2023

Uh oh!

nodejs-github-bot commented Oct 17, 2023

Uh oh!

tniessen commented Oct 17, 2023

Uh oh!

nodejs-github-bot commented Oct 17, 2023

Uh oh!

tniessen commented Oct 17, 2023

Uh oh!

Uzlopak commented Oct 17, 2023

Uh oh!

tniessen commented Oct 17, 2023

Uh oh!

Trott commented Oct 17, 2023

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ronag commented Sep 29, 2023 •

edited

Loading