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: fix memory usage regression in writable #53188

Merged
merged 1 commit into from
Jun 2, 2024
Merged

stream: fix memory usage regression in writable #53188

merged 1 commit into from
Jun 2, 2024

Conversation

orgads
Copy link
Contributor

@orgads orgads commented May 28, 2024 •
edited
Loading

Setting writecb and afterWriteTickInfo to null did not clear the value in the state object.

Amends 35ec931 (stream: writable state bitmap).

Fixes #52228.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 28, 2024
@orgads
Copy link
Contributor Author

orgads commented May 28, 2024

@ronag @mcollina

I'm not sure how to add a test for this. Hints will be welcome :)

I tested with the example app from #52228 and it seems to solve the issue.

@orgads
Copy link
Contributor Author

orgads commented May 28, 2024

Actually the condition is equivalent. I'll need to dig deeper.

@orgads
Copy link
Contributor Author

orgads commented May 28, 2024

@ronag I tried to inspect that change, but couldn't find a smoking gun. #52228 is definitely caused by it, I bisected and triple-checked. Can you try to find the culprit?

I'm closing this PR, as it is wrong anyway.

@orgads orgads closed this May 28, 2024
@orgads orgads reopened this May 29, 2024
@orgads orgads force-pushed the writable-temp-leak branch from eb34ffd to 7b9f16b Compare May 29, 2024 05:53
@orgads orgads changed the title writable: Fix condition mis-translation stream: fix memory leaks in writable May 29, 2024
@orgads orgads force-pushed the writable-temp-leak branch from 7b9f16b to 24b2499 Compare May 29, 2024 05:54
@orgads
Copy link
Contributor Author

orgads commented May 29, 2024

Ok, I found the culprit. Ready for review.

Can you suggest how to add a test?

@orgads orgads mentioned this pull request May 29, 2024
Closed
@benjamingr
Copy link
Member

Can you suggest how to add a test?

Good question, I'm honestly not so sure - you can test writecb is set to null in onwrite but I'm not sure that tests something useful. We don't have memory benchmarks I'm aware of though that might be a good idea overall.

mcollina
mcollina approved these changes May 29, 2024
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

However this is not a memory leak, because the memory consumption is stable.
I would recommend we change the commit/PR title to "stream: fix memory usage regression in writable".

@mcollina
Copy link
Member

A test for this would be nice but I'm at a loss on how to write it.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@orgads orgads force-pushed the writable-temp-leak branch from 24b2499 to aec133f Compare May 29, 2024 14:33
@orgads orgads changed the title stream: fix memory leaks in writable stream: fix memory usage regression in writable May 29, 2024
@orgads
Copy link
Contributor Author

orgads commented May 29, 2024

However this is not a memory leak, because the memory consumption is stable. I would recommend we change the commit/PR title to "stream: fix memory usage regression in writable".

Done.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@orgads
stream: fix memory usage regression in writable
61da0ed 
Setting writecb and afterWriteTickInfo to null did not clear the value
in the state object.

Amends 35ec931 (stream: writable state bitmap).

Fixes nodejs#52228.
@orgads orgads force-pushed the writable-temp-leak branch from aec133f to 61da0ed Compare May 30, 2024 09:55
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 30, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@orgads
Copy link
Contributor Author

orgads commented May 30, 2024

smartos build fails, unrelated to my changes.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 2, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit 78a326e into nodejs:main Jun 2, 2024
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 78a326e

@orgads orgads deleted the writable-temp-leak branch June 2, 2024 01:38
@orgads
Copy link
Contributor Author

orgads commented Jun 2, 2024

Thanks! What's the process of backporting? Should I cherry pick and create a new PR for 22 and 20?

@H4ad
Copy link
Member

H4ad commented Jun 2, 2024

@orgads Since we didn't put labels like "do not land", this PR is automatically eligible to be included in older versions, just wait for the next version, the releasers will include this PR.

You only need to create backport when releasers find a conflict that doesn't allow them merge it easily.

/cc @nodejs/releasers Just to let you know guys an important fix for memory usage on streams was merged.

targos pushed a commit that referenced this pull request Jun 3, 2024
@orgads @targos
stream: fix memory usage regression in writable
a656cf6 
Setting writecb and afterWriteTickInfo to null did not clear the value
in the state object.

Amends 35ec931 (stream: writable state bitmap).

Fixes #52228.

PR-URL: #53188
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jun 7, 2024
Merged
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
@orgads @eliphazbouye
stream: fix memory usage regression in writable
9260946 
Setting writecb and afterWriteTickInfo to null did not clear the value
in the state object.

Amends 35ec931 (stream: writable state bitmap).

Fixes nodejs#52228.

PR-URL: nodejs#53188
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@orgads
Copy link
Contributor Author

orgads commented Jun 21, 2024

@H4ad @marco-ippolito It hasn't been backported to 20.x. :(

@marco-ippolito
Copy link
Member

@H4ad @marco-ippolito It hasn't been backported to 20.x. :(

It has not been on the Current enough (2 weeks) to be backported to v20

@orgads
Copy link
Contributor Author

orgads commented Jun 21, 2024

I see. Thanks for the explanation.

bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@orgads @bmeck
stream: fix memory usage regression in writable
2445d8d 
Setting writecb and afterWriteTickInfo to null did not clear the value
in the state object.

Amends 35ec931 (stream: writable state bitmap).

Fixes nodejs#52228.

PR-URL: nodejs#53188
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@orgads @marco-ippolito
stream: fix memory usage regression in writable
a7bbce9 
Setting writecb and afterWriteTickInfo to null did not clear the value
in the state object.

Amends 35ec931 (stream: writable state bitmap).

Fixes #52228.

PR-URL: #53188
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@orgads @marco-ippolito
stream: fix memory usage regression in writable
324070c 
Setting writecb and afterWriteTickInfo to null did not clear the value
in the state object.

Amends 35ec931 (stream: writable state bitmap).

Fixes #52228.

PR-URL: #53188
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@H4ad H4ad H4ad approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
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.

Significant memory usage increase starting from 20.10.0 for a simple HTTP server (potentially stream related)
7 participants
@orgads @nodejs-github-bot @benjamingr @mcollina @H4ad @marco-ippolito @lpinca