stream: lazy allocate back pressure buffer by ronag · Pull Request #50013 · nodejs/node

ronag · 2023-10-02T11:57:50Z

streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000              *      7.48 %       ±6.60%  ±8.78% ±11.43%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000           ***      8.19 %       ±3.39%  ±4.51%  ±5.87%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000                   -1.00 %       ±3.29%  ±4.43%  ±5.86%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000          ***     12.17 %       ±2.83%  ±3.76%  ±4.90%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000             *      3.61 %       ±3.15%  ±4.24%  ±5.60%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000          ***     13.57 %       ±4.37%  ±5.87%  ±7.77%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000                   1.32 %       ±7.77% ±10.34% ±13.45%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000         ***     21.97 %       ±6.29%  ±8.38% ±10.93%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000                    4.20 %       ±7.15%  ±9.53% ±12.43%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000                   4.54 %       ±5.10%  ±6.87%  ±9.11%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000          ***      5.74 %       ±2.37%  ±3.15%  ±4.11%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000         ***      9.26 %       ±4.66%  ±6.22%  ±8.11%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000            *      4.16 %       ±3.80%  ±5.10%  ±6.72%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000         ***     11.19 %       ±4.06%  ±5.44%  ±7.16%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000           *      4.35 %       ±4.12%  ±5.52%  ±7.28%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000          *      5.89 %       ±4.74%  ±6.35%  ±8.35%

nodejs-github-bot · 2023-10-02T11:57:54Z

Review requested:

@nodejs/streams

lib/internal/streams/writable.js

PR-URL: nodejs#50013

mcollina

lgtm

nodejs-github-bot · 2023-10-02T12:29:43Z

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

lib/internal/streams/writable.js

benjamingr

I think we need to deal with cases the consumer calls .buffered.length as we see from http2, so we can probably define a prototype getter for it that returns it if not initialised and internally use a different property name

PR-URL: nodejs#50013

nodejs-github-bot · 2023-10-03T07:00:33Z

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

lib/internal/http2/core.js

lib/internal/streams/writable.js

nodejs-github-bot · 2023-10-03T07:51:09Z

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

nodejs-github-bot · 2023-10-04T12:01:20Z

Commit Queue failed

- Loading data for nodejs/node/pull/50013
✔  Done loading data for nodejs/node/pull/50013
----------------------------------- PR info ------------------------------------
Title      stream: lazy allocate back pressure buffer (#50013)
Author     Robert Nagy  (@ronag)
Branch     ronag:lazy-buffer -> nodejs:main
Labels     author ready, needs-ci
Commits    3
 - stream: lazy allocate back pressure buffer
 - Update lib/internal/http2/core.js
 - Update lib/internal/streams/writable.js
Committers 2
 - Robert Nagy 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/50013
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50013
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 02 Oct 2023 11:57:50 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/50013#pullrequestreview-1652639906
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/50013#pullrequestreview-1654890641
   ⚠  This PR has conflicts that must be resolved
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-10-03T10:34:32Z: https://ci.nodejs.org/job/node-test-pull-request/54490/
- Querying data for job/node-test-pull-request/54490/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6405634162

PR-URL: nodejs#50013

nodejs-github-bot · 2023-10-04T14:36:55Z

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

nodejs-github-bot · 2023-10-04T17:22:27Z

Commit Queue failed

- Loading data for nodejs/node/pull/50013
✔  Done loading data for nodejs/node/pull/50013
----------------------------------- PR info ------------------------------------
Title      stream: lazy allocate back pressure buffer (#50013)
Author     Robert Nagy  (@ronag)
Branch     ronag:lazy-buffer -> nodejs:main
Labels     author ready, needs-ci
Commits    1
 - stream: lazy allocate back pressure buffer
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/50013
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50013
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - stream: lazy allocate back pressure buffer
   ℹ  This PR was created on Mon, 02 Oct 2023 11:57:50 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/50013#pullrequestreview-1652639906
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/50013#pullrequestreview-1654890641
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-10-04T14:36:55Z: https://ci.nodejs.org/job/node-test-pull-request/54529/
- Querying data for job/node-test-pull-request/54529/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6409353418

liuxingbaoyu · 2023-10-04T18:16:33Z

lib/internal/streams/writable.js

+    if ((state.state & kBuffered) === 0) {
+      state.state |= kBuffered;
+      state[kBufferedValue] = [];
+    }
+
+    state[kBufferedValue].push({ chunk, encoding, callback });


if ((state.state & kBuffered) === 0) { state.state |= kBuffered; state[kBufferedValue] = [{ chunk, encoding, callback }]; } else { state[kBufferedValue].push({ chunk, encoding, callback }); }

Just curious, will there be any observable performance improvements?

nodejs-github-bot · 2023-10-05T18:40:10Z

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

lpinca

RSLGTM

nodejs-github-bot · 2023-10-05T20:27:47Z

Landed in e9bda11

PR-URL: nodejs#50013 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #50013 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs#50013 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

ronag requested review from benjamingr and mcollina October 2, 2023 11:57

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

ronag force-pushed the lazy-buffer branch 3 times, most recently from 91605c3 to fe4064a Compare October 2, 2023 12:09

MoLow reviewed Oct 2, 2023

View reviewed changes

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

ronag force-pushed the lazy-buffer branch from fe4064a to d46c603 Compare October 2, 2023 12:18

ronag added a commit to nxtedition/node that referenced this pull request Oct 2, 2023

stream: lazy allocate back pressure buffer

f94fef0

PR-URL: nodejs#50013

ronag force-pushed the lazy-buffer branch from d46c603 to f94fef0 Compare October 2, 2023 12:18

mcollina approved these changes Oct 2, 2023

View reviewed changes

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

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

benjamingr reviewed Oct 2, 2023

View reviewed changes

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

benjamingr approved these changes Oct 2, 2023

View reviewed changes

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

CI Reliability 2023-10-03 nodejs/reliability#682

Open

18 tasks

ronag added a commit to nxtedition/node that referenced this pull request Oct 3, 2023

stream: lazy allocate back pressure buffer

6654c29

PR-URL: nodejs#50013

ronag force-pushed the lazy-buffer branch from f94fef0 to 6654c29 Compare October 3, 2023 06:59

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

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

ronag commented Oct 3, 2023

View reviewed changes

lib/internal/http2/core.js Outdated Show resolved Hide resolved

ronag commented Oct 3, 2023

View reviewed changes

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

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

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

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

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

ronag added a commit to nxtedition/node that referenced this pull request Oct 4, 2023

stream: lazy allocate back pressure buffer

5ef5226

PR-URL: nodejs#50013

stream: lazy allocate back pressure buffer

2ca6892

PR-URL: nodejs#50013

ronag force-pushed the lazy-buffer branch from b6bb531 to 5ef5226 Compare October 4, 2023 14:33

ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 4, 2023

ronag force-pushed the lazy-buffer branch from 5ef5226 to 2ca6892 Compare October 4, 2023 14:33

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

ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 4, 2023

liuxingbaoyu reviewed Oct 4, 2023

View reviewed changes

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

CI Reliability 2023-10-05 nodejs/reliability#684

Open

13 tasks

ronag removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 5, 2023

lpinca approved these changes Oct 5, 2023

View reviewed changes

ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2023

nodejs-github-bot merged commit e9bda11 into nodejs:main Oct 5, 2023

This was referenced Oct 6, 2023

CI Reliability 2023-10-06 nodejs/reliability#685

Open

CI Reliability 2023-10-09 nodejs/reliability#688

Open

RafaelGSS mentioned this pull request Oct 9, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

targos pushed a commit that referenced this pull request Nov 11, 2023

stream: lazy allocate back pressure buffer

ecbfb23

PR-URL: #50013 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

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 Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 2, 2023

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 2, 2023

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 3, 2023

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 3, 2023

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

liuxingbaoyu Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 5, 2023

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ronag commented Oct 2, 2023 •

edited

Loading