GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — stream: lazy allocate back pressure buffer by ronag · Pull Request #50013 · nodejs/node

Conversation

ronag commented Oct 2, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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%

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow, bricss, H4ad, illusi0n7, and daniellockyer reacted with rocket emoji

All reactions

🚀 5 reactions

ronag requested review from benjamingr and mcollina

October 2, 2023 11:57

nodejs-github-bot commented Oct 2, 2023

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/streams

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

lib/internal/streams/writable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ronag force-pushed the lazy-buffer branch from fe4064a to d46c603 Compare

October 2, 2023 12:18

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

mcollina left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Oct 2, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Oct 2, 2023

nodejs-github-bot commented Oct 2, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr reviewed

Oct 2, 2023

View reviewed changes

Comment thread

lib/internal/streams/writable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr approved these changes

Oct 2, 2023

View reviewed changes

benjamingr left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot mentioned this pull request

Oct 3, 2023

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

Open

18 tasks

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. Only starts once the PR has an approving review. label

Oct 3, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Oct 3, 2023

nodejs-github-bot commented Oct 3, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ronag commented

Oct 3, 2023

View reviewed changes

Comment thread

lib/internal/http2/core.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ronag commented

Oct 3, 2023

View reviewed changes

Comment thread

lib/internal/streams/writable.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Oct 3, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Oct 3, 2023

nodejs-github-bot commented Oct 3, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Oct 3, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Oct 3, 2023

nodejs-github-bot commented Oct 4, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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. Only starts once the PR has an approving review. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. 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. Only starts once the PR has an approving review. label

Oct 4, 2023

nodejs-github-bot commented Oct 4, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ronag added the commit-queue PRs queued for automated landing through the Commit Queue. label

Oct 4, 2023

nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels

Oct 4, 2023

nodejs-github-bot commented Oct 4, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

liuxingbaoyu reviewed

Oct 4, 2023

View reviewed changes

Comment thread

lib/internal/streams/writable.js

Comment on lines +537 to +542

		if ((state.state & kBuffered) === 0) {
		state.state \|= kBuffered;
		state[kBufferedValue] = [];
		}

		state[kBufferedValue].push({ chunk, encoding, callback });

liuxingbaoyu Oct 4, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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 PRs whose Commit Queue landing failed and need manual intervention before retrying. label

Oct 5, 2023

nodejs-github-bot commented Oct 5, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca approved these changes

Oct 5, 2023

View reviewed changes

lpinca left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

RSLGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ronag added the commit-queue PRs queued for automated landing through the Commit Queue. label

Oct 5, 2023

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Oct 5, 2023

nodejs-github-bot merged commit e9bda11 into nodejs:main

Oct 5, 2023

nodejs-github-bot commented Oct 5, 2023

Copy link

Copy Markdown

Collaborator

Landed in e9bda11

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>

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

debadree25 pushed a commit to debadree25/node that referenced this pull request

Apr 15, 2024


          stream: lazy allocate back pressure buffer

e7a7779

PR-URL: nodejs#50013
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>

Reviewers

MoLow

MoLow left review comments

mcollina

mcollina approved these changes

benjamingr

benjamingr approved these changes

lpinca

lpinca approved these changes

+1 more reviewer

liuxingbaoyu

liuxingbaoyu left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

needs-ci

PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ronag commented Oct 2, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.