stream: fix Writable.destroy performance regression#50409
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
Collaborator
Review requested:
|
ronagforce-pushed the writable-destroy-pref branch from fe83419 to c3403b4Compare
rluvaton approved these changes Oct 26, 2023
ronag added request-ci 
Uzlopak approved these changes Oct 26, 2023
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Oct 26, 2023
rluvaton added the performance 
benjamingr approved these changes Oct 26, 2023
19 tasks
Collaborator
nodejs-github-bot commented Oct 27, 2023
Collaborator
nodejs-github-bot commented Oct 27, 2023
ronag added the commit-queue 22 tasks
nodejs-github-bot removed the commit-queue Collaborator
nodejs-github-bot commented Oct 28, 2023
Commit Queue failed- Loading data for nodejs/node/pull/50409 ✔ Done loading data for nodejs/node/pull/50409 ----------------------------------- PR info ------------------------------------ Title stream: fix Writable.destroy performance regression (#50409) Author Robert Nagy (@ronag) Branch ronag:writable-destroy-pref -> nodejs:main Labels performance, author ready, needs-ci Commits 1 - stream: fix Writable.destroy performance regression Committers 1 - Robert Nagy PR-URL: https://github.com/nodejs/node/pull/50409 Reviewed-By: Raz Luvaton Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50409 Reviewed-By: Raz Luvaton Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 26 Oct 2023 13:53:21 GMT ✔ Approvals: 3 ✔ - Raz Luvaton (@rluvaton): https://github.com/nodejs/node/pull/50409#pullrequestreview-1699727857 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/50409#pullrequestreview-1699783397 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/50409#pullrequestreview-1700754946 ✘ Last GitHub CI failed ℹ Last Benchmark CI on 2023-10-26T13:58:47Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1460/ ℹ Last Full PR CI on 2023-10-27T10:42:36Z: https://ci.nodejs.org/job/node-test-pull-request/55266/ - Querying data for job/node-test-pull-request/55266/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/6677348142 |
nodejs-github-bot added the commit-queue-failed 
jasnell approved these changes Oct 28, 2023
ronag added commit-queue nodejs-github-bot removed the commit-queue Collaborator
nodejs-github-bot commented Oct 28, 2023
Commit Queue failed- Loading data for nodejs/node/pull/50409 ✔ Done loading data for nodejs/node/pull/50409 ----------------------------------- PR info ------------------------------------ Title stream: fix Writable.destroy performance regression (#50409) Author Robert Nagy (@ronag) Branch ronag:writable-destroy-pref -> nodejs:main Labels performance, author ready, needs-ci Commits 1 - stream: fix Writable.destroy performance regression Committers 1 - Robert Nagy PR-URL: https://github.com/nodejs/node/pull/50409 Reviewed-By: Raz Luvaton Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50409 Reviewed-By: Raz Luvaton Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 26 Oct 2023 13:53:21 GMT ✔ Approvals: 4 ✔ - Raz Luvaton (@rluvaton): https://github.com/nodejs/node/pull/50409#pullrequestreview-1699727857 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/50409#pullrequestreview-1699783397 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/50409#pullrequestreview-1700754946 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/50409#pullrequestreview-1702808873 ✘ Last GitHub CI failed ℹ Last Benchmark CI on 2023-10-28T14:03:30Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1460/ ℹ Last Full PR CI on 2023-10-28T14:03:30Z: https://ci.nodejs.org/job/node-test-pull-request/55266/ - Querying data for job/node-test-pull-request/1460/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/6678407207 |
nodejs-github-bot added the commit-queue-failed This was referenced Oct 29, 2023
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 30, 2023
Member
Trott commented Oct 30, 2023
For whatever reason, this is failing the coverage-without-intl job over and over on GitHub Actions. I applied this change on top of current main (preserving commit authorship) and that passed over in #50478. So I'll run a full CI there. If the folks who approved this PR could also approve that one so it can land (and this one can be closed), that would be great. |
17 tasks
Member
Trott commented Oct 31, 2023
#50478 is exactly this PR but rebased. CI is passing on that PR and it is ready to land, but needs one more approval. If someone who approved this PR would approve that one, we could add the |
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 31, 2023
Ref: nodejs#50409 PR-URL: nodejs#50478 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
Ref: nodejs#50409 PR-URL: nodejs#50478 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
anonrig pushed a commit to anonrig/node that referenced this pull request Nov 9, 2023
Ref: nodejs#50409 PR-URL: nodejs#50478 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
Ref: #50409 PR-URL: #50478 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Nov 14, 2023
Ref: #50409 PR-URL: #50478 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
Ref: #50409 PR-URL: #50478 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
8 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.
No description provided.