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.

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

Fix memory leaks of jest-worker #11187

Merged
merged 5 commits into from
Mar 15, 2021
Merged

Fix memory leaks of jest-worker #11187

merged 5 commits into from
Mar 15, 2021

Conversation

shuding
Copy link
Contributor

@shuding shuding commented Mar 12, 2021

Summary

We noticed some closure related memory leaks in the current implementation of jest-worker with v8. The arguments and result of the task of each worker can't be garbage collected until a new task is assigned to that worker. If the arguments/result sizes are large, it's gonna introduce a huge memory increase.

This PR ensures that args and task are correctly scoped so they won't be claimed by other closures, and references can be cleaned once they become unneeded.

Test plan

We have an app that passes image buffers as the argument of tasks which are pretty large, and we are seeing a huge memory leak after the tasks have finished (ref: vercel/next.js#22925). Here's a screenshot for the memory dump:

CleanShot 2021-03-13 at 02 29 23

The memory usage increased from 112MB to 145MB (+29%) after processing the task, and most of the new allocated memory was taken by the arguments and result of that task, which should have been recollected after it ends.

With this fix we are no longer to reproduce this leak with the same app:

image

@netlify
Copy link

netlify bot commented Mar 12, 2021

Deploy preview for jestjs ready!

Built without sensitive environment variables with commit 698e78e

https://deploy-preview-11187--jestjs.netlify.app

@shuding shuding marked this pull request as ready for review March 12, 2021 19:12
@netlify
Copy link

netlify bot commented Mar 12, 2021

Deploy preview for jestjs ready!

Built without sensitive environment variables with commit 852ec1c

https://deploy-preview-11187--jestjs.netlify.app

@SimenB SimenB requested a review from MichaReiser March 12, 2021 21:09
@SimenB SimenB assigned jeysal and unassigned jeysal Mar 12, 2021
@SimenB SimenB requested a review from jeysal March 12, 2021 21:09
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

First of all, thanks a lot for tracking down the source and finding a fix for this!
I've added a regression test that fails on master but passes with your fix (pretty cool how easy to test this is with the leak detector IMO), and moved the changelog entry do the perf section where I think it fits better.
Let me know if you are ok with the changes :)

@shuding
Copy link
Contributor Author

shuding commented Mar 15, 2021

Thank you for adding the test @jeysal, it looks great!

@jeysal
Copy link
Contributor

jeysal commented Mar 15, 2021

Great spot and thanks again!

@jeysal jeysal merged commit 427af3b into jestjs:master Mar 15, 2021
@shuding shuding deleted the fix-worker-memory-leak branch March 15, 2021 12:30
@SimenB
Copy link
Member

SimenB commented Mar 15, 2021

@MichaReiser FYI

@SimenB
Copy link
Member

SimenB commented Mar 15, 2021

@shuding would a pre-release of this help? If not urgent a stable release should be out this month (🤞🤞🤞)

@shuding
Copy link
Contributor Author

shuding commented Mar 15, 2021

@SimenB Yep we're good with a pre-release, thanks for your help!

@SimenB
Copy link
Member

SimenB commented Mar 15, 2021

jest-worker@27.0.0-next.5 is available now 👍

kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Mar 16, 2021
This PR upgrades `jest-worker` and `jest-cli` to the latest pre-release version, also removed `jest-circus` which is included in Jest by default. `jest-worker@next` includes a fix for memory leak that we need (jestjs/jest#11187). 

Fixes #22925. This will also improve the OOM issue for `next dev` #15855.
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Apr 29, 2021
This PR upgrades `jest-worker` and `jest-cli` to the latest pre-release version, also removed `jest-circus` which is included in Jest by default. `jest-worker@next` includes a fix for memory leak that we need (jestjs/jest#11187). 

Fixes vercel#22925. This will also improve the OOM issue for `next dev` vercel#15855.
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants