Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: improve Writable.destroy performance #35067

Closed
wants to merge 4 commits into from
Closed

stream: improve Writable.destroy performance #35067

wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 5, 2020

Avoid nextTick if there are no pending callbacks.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag
stream: improbe Writable.destroy performance
6734876 
Avoid nextTick if there are no pending callbacks.
@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Sep 5, 2020
@ronag
Copy link
Member Author

ronag commented Sep 5, 2020

@nodejs/streams

mcollina
mcollina approved these changes Sep 5, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 5, 2020
@ronag ronag changed the title stream: improbe Writable.destroy performance stream: improve Writable.destroy performance Sep 5, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2020
@nodejs-github-bot
Copy link
Collaborator

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

addaleax
addaleax approved these changes Sep 5, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

Feel free to add a comment that this condition is equivalent to no callbacks being pending :)

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 7, 2020
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2020
@github-actions github-actions 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 Sep 7, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35067
✔  Done loading data for nodejs/node/pull/35067
----------------------------------- PR info ------------------------------------
Title      stream: improve Writable.destroy performance (#35067)
Author     Robert Nagy  (@ronag)
Branch     ronag:writable-destroy-perf -> nodejs:master
Labels     author ready, stream
Commits    4
 - stream: improbe Writable.destroy performance
 - fixup
 - fixup
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/35067
Reviewed-By: Matteo Collina 
Reviewed-By: Anna Henningsen 
Reviewed-By: Luigi Pinca 
Reviewed-By: Ricky Zhou <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35067
Reviewed-By: Matteo Collina 
Reviewed-By: Anna Henningsen 
Reviewed-By: Luigi Pinca 
Reviewed-By: Ricky Zhou <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - fixup
   ⚠  - fixup
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-09-07T09:00:43Z: https://ci.nodejs.org/job/node-test-pull-request/33086/
- Querying data for job/node-test-pull-request/33086/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sat, 05 Sep 2020 11:51:48 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35067#pullrequestreview-483051355
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35067#pullrequestreview-483067062
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/35067#pullrequestreview-483072059
   ✔  - Ricky Zhou (@rickyes): https://github.com/nodejs/node/pull/35067#pullrequestreview-483143528
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

@addaleax addaleax 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 Sep 7, 2020
@github-actions github-actions 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 Sep 7, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35067
✔  Done loading data for nodejs/node/pull/35067
----------------------------------- PR info ------------------------------------
Title      stream: improve Writable.destroy performance (#35067)
Author     Robert Nagy  (@ronag)
Branch     ronag:writable-destroy-perf -> nodejs:master
Labels     author ready, stream
Commits    4
 - stream: improbe Writable.destroy performance
 - fixup
 - fixup
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/35067
Reviewed-By: Matteo Collina 
Reviewed-By: Anna Henningsen 
Reviewed-By: Luigi Pinca 
Reviewed-By: Ricky Zhou <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35067
Reviewed-By: Matteo Collina 
Reviewed-By: Anna Henningsen 
Reviewed-By: Luigi Pinca 
Reviewed-By: Ricky Zhou <[email protected]>
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-09-07T14:21:04Z: https://ci.nodejs.org/job/node-test-pull-request/33086/
- Querying data for job/node-test-pull-request/33086/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sat, 05 Sep 2020 11:51:48 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35067#pullrequestreview-483051355
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35067#pullrequestreview-483593618
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/35067#pullrequestreview-483072059
   ✔  - Ricky Zhou (@rickyes): https://github.com/nodejs/node/pull/35067#pullrequestreview-483143528
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

@ronag
Copy link
Member Author

ronag commented Sep 7, 2020

@nodejs/build a little trouble landing with the GH action? Something you want to take a look at or should I just land this manually?

@addaleax addaleax mentioned this pull request Sep 7, 2020
Merged
Trott pushed a commit to Trott/io.js that referenced this pull request Sep 7, 2020
@ronag @Trott
stream: improve Writable.destroy performance
e0d3b75 
Avoid nextTick if there are no pending callbacks.

PR-URL: nodejs#35067
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@Trott
Copy link
Member

Trott commented Sep 7, 2020

Landed in e0d3b75

@Trott Trott closed this Sep 7, 2020
@ljharb
Copy link
Member

ljharb commented Sep 7, 2020 •
edited
Loading

@ronag have had you checked the "allow edits" box on the PR?

@Trott
Copy link
Member

Trott commented Sep 7, 2020

Sorry if I stomped all over the "land manually? change edit permissions? something else?" conversation by landing this. Wasn't my intention. I figured @ronag probably would prefer a resolution (landing) over some back-and-forth about this, but maybe I'm wrong.

@ljharb
Copy link
Member

ljharb commented Sep 7, 2020 •
edited
Loading

@Trott imo it should always be 100% ok to land over this particular X, just trying to figure out the forensics on the implication that it was unavoidable.

@ruyadorno
Copy link
Member

this doesn't land cleanly on v14.x-staging adding a request to backport

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@ronag @joesepi
stream: improve Writable.destroy performance
079af89 
Avoid nextTick if there are no pending callbacks.

PR-URL: nodejs#35067
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
@Linkgoron Linkgoron mentioned this pull request Apr 10, 2021
Closed
@targos targos added dont-land-on-v14.x and removed backport-requested-v14.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickyes rickyes rickyes approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
stream Issues 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
@ronag @nodejs-github-bot @Trott @ljharb @ruyadorno @mcollina @addaleax @lpinca @rickyes @targos